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

LandCover.ai: add GeoDataset version #1126

Merged
merged 13 commits into from
Feb 22, 2023
Merged

LandCover.ai: add GeoDataset version #1126

merged 13 commits into from
Feb 22, 2023

Conversation

adrianboguszewski
Copy link
Contributor

This PR renames LandCoverAI class to LandCoverAINonGeo (benchmark dataset, created with split.py script) and introduces the abstract base LandCoverAI class and LandCoverAIGeo (raster dataset).

This is my proposition, but I am open to the discussion of how the structure, inheritance, and naming should look.

@github-actions github-actions bot added datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets documentation Improvements or additions to documentation labels Feb 19, 2023
@adrianboguszewski
Copy link
Contributor Author

@microsoft-github-policy-service agree

@github-actions github-actions bot added the testing Continuous integration testing label Feb 19, 2023
@adrianboguszewski adrianboguszewski marked this pull request as ready for review February 19, 2023 21:27
@adamjstewart adamjstewart added this to the 0.5.0 milestone Feb 19, 2023
@adamjstewart adamjstewart added the backwards-incompatible Changes that are not backwards compatible label Feb 19, 2023
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.

This looks correct modulo some documentation hiccups. Let me mull over the class names. Some options:

  • LandCoverAINonGeo and LandCoverAIGeo
  • LandCoverAI and LandCoverAIGeo
  • LandCoverAIBenchmark and LandCoverAIUncurated

I think this will be the first dataset where we have both a geo and non-geo version, so we have an opportunity to set a new standard naming scheme.

torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
docs/api/datasets.rst Outdated Show resolved Hide resolved
docs/api/datasets.rst Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Show resolved Hide resolved
@calebrob6
Copy link
Member

For naming, I prefer something like LandCoverAI for the GeoDataset and LandCoverAIFixed / LandCoverAIPatched for the NonGeoDataset.

@adamjstewart
Copy link
Collaborator

I think @isaaccorley said he prefers LandCoverAI for non-geo and LandCoverAIGeo for geo. Not sure how to come to agreement on the names. Would be great if we can decide on an approach that is consistent for all datasets, there are many others that could be both geo and non-geo.

@calebrob6
Copy link
Member

Yeah on second thought I like that (LandCoverAI for non-geo and LandCoverAIGeo for geo) a bit better:

  • Doesn't break existing code
  • It makes the non-geo dataset seem like the default (which is likely what people want if they're just trying to compare to published numbers)

@adamjstewart
Copy link
Collaborator

The base class can be called LandCoverAIBase to disambiguate

@adrianboguszewski
Copy link
Contributor Author

Thank you guys for the review and a valuable name discussion. backwards-incompatible label can be removed.

@github-actions github-actions bot removed the datamodules PyTorch Lightning datamodules label Feb 20, 2023
@adamjstewart adamjstewart removed the backwards-incompatible Changes that are not backwards compatible label Feb 21, 2023
docs/api/geo_datasets.csv Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Outdated Show resolved Hide resolved
torchgeo/datasets/landcoverai.py Show resolved Hide resolved
@adamjstewart adamjstewart changed the title LandCover.ai updates LandCover.ai: add GeoDataset version Feb 21, 2023
adrianboguszewski and others added 2 commits February 21, 2023 19:43
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@adamjstewart adamjstewart merged commit 20ee20c into microsoft:main Feb 22, 2023
@adrianboguszewski adrianboguszewski deleted the landcover-updates branch February 23, 2023 10:50
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Added LandCover.ai geo and non-geo datasets

* Updated documentation

* Using the new classes in tests

* Fixes for documentation

* Added tests for geo dataset

* Improvements according to the review

* Small fixes

* Documentation fixes

* Small documentation fixes

* Update docs/api/geo_datasets.csv

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

* Fix missing line of coverage

---------

Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants