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

Add own data to CDL dataset. #429

Merged
merged 3 commits into from
Feb 25, 2022
Merged

Add own data to CDL dataset. #429

merged 3 commits into from
Feb 25, 2022

Conversation

nilsleh
Copy link
Collaborator

@nilsleh nilsleh commented Feb 23, 2022

As requested in #415 , this PR adds a data.py and new fake data.

@github-actions github-actions bot added the testing Continuous integration testing label Feb 23, 2022
@calebrob6
Copy link
Member

calebrob6 commented Feb 23, 2022

Hmmm, I think we should add an explicit test for the cmap to get around this test coverage thing.

Update: still not quite sure why test coverage would drop, but #430 should fix this.

@adamjstewart
Copy link
Collaborator

Update: still not quite sure why test coverage would drop, but #430 should fix this.

Coverage is dropping because the new fake data does not have a cmap. We should add this to the data. Also, it seems like the actual data.py is missing from this PR.

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Feb 24, 2022
@adamjstewart adamjstewart added this to the 0.2.1 milestone Feb 24, 2022
@calebrob6 calebrob6 merged commit 4bf48cf into microsoft:main Feb 25, 2022
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.

Few more things that can be improved in a follow-up PR

@@ -58,6 +58,8 @@ class CDL(RasterDataset):
(2008, "e6aa3967e379b98fd30c26abe9696053"),
]

cmap: Dict[int, Tuple[int, int, int, int]] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already defined at the RasterDataset level, I don't think we need it here too

@@ -31,9 +31,21 @@ def dataset(
monkeypatch.setattr( # type: ignore[attr-defined]
torchgeo.datasets.cdl, "download_url", download_url
)
cmap = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This cmap should be added directly to the file so we can test that TorchGeo can read the cmap from a file. See https://github.com/microsoft/torchgeo/tree/main/tests/data#raster-data for the code to add this to the file.



directories = ["2020_30m_cdls", "2021_30m_cdls"]
raster_extensions = [[".tif", ".tif.ovr"], [".tif", ".tif.ovr"]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably simplify this to a single list, no need to duplicate it

adamjstewart pushed a commit that referenced this pull request Mar 19, 2022
* add own data

* data.py with cmap

* remove .img test data format
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* add own data

* data.py with cmap

* remove .img test data format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants