Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LEVIR-CD Dataset and Datamodule #1770

Merged
merged 10 commits into from
Dec 17, 2023
Merged

LEVIR-CD Dataset and Datamodule #1770

merged 10 commits into from
Dec 17, 2023

Conversation

isaaccorley
Copy link
Collaborator

This PR adds the LEVIR-CD dataset and datamodule.

This dataset is just the prior version of LEVIR-CD+ with fewer images. It also has a slightly different directory structure.

Also refactored the datasets to inherit from a general base LEVIRCD class

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing datamodules PyTorch Lightning datamodules labels Dec 13, 2023
@isaaccorley isaaccorley self-assigned this Dec 13, 2023
nilsleh
nilsleh requested changes Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@nilsleh nilsleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the docs also need to be updated with the new addition datasets.

tests/datamodules/test_levircd.py Outdated Show resolved Hide resolved
tmp.ipynb Outdated Show resolved Hide resolved
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 13, 2023
nilsleh
nilsleh reviewed Dec 15, 2023
View reviewed changes
Copy link
Collaborator

@nilsleh nilsleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

docs/api/datamodules.rst Outdated Show resolved Hide resolved
docs/api/datasets.rst Outdated Show resolved Hide resolved
torchgeo/datasets/levircd.py Show resolved Hide resolved
@isaaccorley isaaccorley enabled auto-merge (squash) December 17, 2023 03:41
@isaaccorley isaaccorley mentioned this pull request Dec 17, 2023
Closed
@isaaccorley isaaccorley merged commit 46f4be7 into microsoft:main Dec 17, 2023
21 checks passed
@adamjstewart adamjstewart added this to the 0.6.0 milestone Dec 17, 2023
adamjstewart
adamjstewart reviewed Dec 17, 2023
View reviewed changes
docs/api/datamodules.rst
@@ -85,6 +85,16 @@ LandCover.ai

.. autoclass:: LandCoverAIDataModule

LEVIR-CD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prob just have a single header with 2 versions

docs/api/datasets.rst
@@ -267,6 +267,12 @@ LandCover.ai

.. autoclass:: LandCoverAI

LEVIR-CD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

docs/api/non_geo_datasets.csv
@@ -18,6 +18,7 @@ Dataset,Task,Source,License,# Samples,# Classes,Size (px),Resolution (m),Bands
`IDTReeS`_,"OD,C",Aerial,"CC-BY-4.0",591,33,200x200,0.1--1,RGB
`Inria Aerial Image Labeling`_,S,Aerial,-,360,2,"5,000x5,000",0.3,RGB
`LandCover.ai`_,S,Aerial,"CC-BY-NC-SA-4.0","10,674",5,512x512,0.25--0.5,RGB
`LEVIR-CD`_,CD,Google Earth,-,637,2,"1,024x1,024",0.5,RGB
`LEVIR-CD+`_,CD,Google Earth,-,985,2,"1,024x1,024",0.5,RGB
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could prob just combine

tests/datamodules/test_levircd.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file will eventually be removed once we have a trainer for CD

torchgeo/datasets/levircd.py
Comment on lines +233 to +234
1. no change
2. change
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does our __getitem__ remap from 0, 255 to 0, 1? Should it? We should label the classes with the returned value, not the stored value.

torchgeo/datasets/levircd.py
Comment on lines +311 to +312
The `LEVIR-CD+ <https://github.com/S2Looking/Dataset>`__
dataset is a dataset for building change detection.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change all:

The foo dataset is a dataset for bar

to:

Foo is a bar dataset

It just reads better.

torchgeo/datasets/levircd.py

* https://arxiv.org/abs/2107.09244

.. versionchanged:: 0.6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this not be true? We already had a LEVIRCDPlus class and this PR changed it. There are no breaking changes to it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I read this as versionadded the first time. Still, we should say what changed. Or did the API not change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that in another PR. I just made a base class so I could override a couple methods for loading the files since they have different directory setups. The LEVIRCDPlus itself hasn't changed though other than inheriting from the new base class

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I would remove versionchanged, there are no significant API differences that users need to be aware of.

torchgeo/datasets/levircd.py
filename=self.filename,
md5=self.md5 if self.checksum else None,
)

def plot(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example plot for both versions?

torchgeo/datamodules/levircd.py
Comment on lines +51 to +57
self.val_aug = AugmentationSequential(
K.Normalize(mean=self.mean, std=self.std),
data_keys=["image1", "image2", "mask"],
)
self.test_aug = AugmentationSequential(
K.Normalize(mean=self.mean, std=self.std),
data_keys=["image1", "image2", "mask"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set self.aug you don't need to duplicate this

@isaaccorley isaaccorley deleted the datasets/levircd branch December 19, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamjstewart adamjstewart adamjstewart left review comments

@robmarkcole robmarkcole robmarkcole left review comments

@nilsleh nilsleh nilsleh approved these changes

@calebrob6 calebrob6 Awaiting requested review from calebrob6

Assignees

@isaaccorley isaaccorley

Labels
datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets documentation Improvements or additions to documentation testing Continuous integration testing
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@isaaccorley @robmarkcole @adamjstewart @nilsleh