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.

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

EuroSAT Subclass with Longitude-Based Splits #2074

Merged
merged 29 commits into from
May 29, 2024
Merged

EuroSAT Subclass with Longitude-Based Splits #2074

merged 29 commits into from
May 29, 2024

Conversation

burakekim
Copy link
Contributor

SpatialSplitEuroSAT subclass overriding the default splits of EuroSAT.

Splits 60/20/20 by longitude.

Kudos to @calebrob6 for creating the spatial splits.

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label May 21, 2024
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

We haven't really thought about the best way to handle multiple split definitions for a single dataset. Do we want to create a new class, or just a new parameter in the original class?

torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
@adamjstewart adamjstewart added this to the 0.6.0 milestone May 21, 2024
@burakekim
Copy link
Contributor Author

burakekim commented May 21, 2024

We haven't really thought about the best way to handle multiple split definitions for a single dataset. Do we want to create a new class, or just a new parameter in the original class?

No preference. Adding a new parameter to the original class might keep things simpler and cut down on extra code though

@burakekim
Copy link
Contributor Author

On a side note, it might make sense to give the users heads up that they need to manually set the stage in setup method

dataset = SpatialSplitEuroSAT(root=data_root, split="test", transforms=test_transforms_gpu)
dataloader = SpatialSplitEuroSATDataModule(root=data_root, batch_size=1)

# Setup the datasets for the 'test' stage
dataloader.setup(stage="test")

@adamjstewart
Copy link
Collaborator

On a side note, it might make sense to give the users heads up that they need to manually set the stage in setup method

That's something that can be documented in Lightning. I don't actually expect users to use the data module without a Trainer, which handles all of this for you.

torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Collaborator

Also needs tests. Should be as simple as adding one more class name here, one config file similar to here, and one new line here.

@github-actions github-actions bot added testing Continuous integration testing datamodules PyTorch Lightning datamodules labels May 26, 2024
@burakekim
Copy link
Contributor Author

Also needs tests. Should be as simple as adding one more class name here, one config file similar to here, and one new line here.

Thanks for pointing out these! Done. Hope it is good now

Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

torchgeo/datamodules/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datamodules/eurosat.py Outdated Show resolved Hide resolved
torchgeo/datasets/eurosat.py Outdated Show resolved Hide resolved
burakekim and others added 2 commits May 28, 2024 14:26
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 28, 2024
@adamjstewart
Copy link
Collaborator

Add to torchgeo.datamodules.__init__.py to solve the readthedocs error.

adamjstewart
adamjstewart previously approved these changes May 29, 2024
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Optional: we could put Spatial before 100 in the code and docs since users aren't really giong to use 100, it's only for demos. But it's prob fine to merge as is.

@burakekim
Copy link
Contributor Author

Optional: we could put Spatial before 100 in the code and docs since users aren't really giong to use 100, it's only for demos. But it's prob fine to merge as is.

No preference: So I can do that!

@adamjstewart
Copy link
Collaborator

Let's do it then.

@burakekim
Copy link
Contributor Author

Done!

@adamjstewart adamjstewart merged commit 1268993 into microsoft:main May 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets documentation Improvements or additions to documentation testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants