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 plot method and data.py to CBF #410

Merged
merged 11 commits into from
Mar 20, 2022
Merged

Conversation

nilsleh
Copy link
Collaborator

@nilsleh nilsleh commented Feb 17, 2022

Since RasterDatasets should have their own plot method per #253, this PR adds a plot method as well as a data.py file with adapted fake data to the the CBF dataset.

Example:
cbf

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing labels Feb 17, 2022
@adamjstewart adamjstewart added this to the 0.3.0 milestone Feb 18, 2022
@calebrob6
Copy link
Member

No idea what is happening with the tests here, let me know if you want me to look more into it @nilsleh

@calebrob6 calebrob6 closed this Feb 20, 2022
@calebrob6 calebrob6 reopened this Feb 20, 2022
@nilsleh
Copy link
Collaborator Author

nilsleh commented Feb 22, 2022

Yes, sorry I am not sure what is happening, locally it passes all tests.

@calebrob6
Copy link
Member

Will this plot method work with a VectorDataset?

@nilsleh
Copy link
Collaborator Author

nilsleh commented Feb 23, 2022

Will this plot method work with a VectorDataset?

I added an example from the cbf dataset.

torchgeo/datasets/cbf.py Outdated Show resolved Hide resolved
adamjstewart
adamjstewart previously approved these changes Feb 26, 2022
@@ -675,21 +675,6 @@ def __getitem__(self, query: BoundingBox) -> Dict[str, Any]:

return sample

def plot(self, data: Tensor) -> None:
Copy link
Collaborator

@adamjstewart adamjstewart Feb 26, 2022

Choose a reason for hiding this comment

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

Since CBF was the only VectorDataset, and since it overrides the plot method, this code was no longer being tested. We have a few options:

  1. Remove VectorDataset.plot
  2. Keep it, update the signature to take in a sample, and use a useful default
  3. Keep it, update the signature to take in a sample, and raise NotImplementedError
  4. Turn it into an abstract method and require all subclasses to implement it

The benefit of 1 is that it is the least code. The benefit of 2-4 is that they add mypy checks to ensure that all subclasses use the same signature. The benefit of 4 is that it prevents someone from adding a new subclass that doesn't implement plot. The downside of 4 is that it means you can't instantiate a VectorDataset directly.

I forget where we discussed this already, but I have a slight preference for 4 because it seems like "the right thing to do", and @calebrob6 has a slight preference for 3 because it allows you to use VectorDataset directly without creating a subclass. I think 1 and 2 are valid options too. Interested in hearing other opinions on this one.

Note that this will also affect RasterDataset once we finish adding individual plot methods to all classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if our plotting code has enough in common that we could define one or two plot methods in VisionDataset and GeoDataset and then have each subclass call super with the appropriate arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anyone have any opinions on this? We should make this decision so we can finish adding plot methods to all GeoDatasets.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if our plotting code has enough in common that we could define one or two plot methods in VisionDataset and GeoDataset and then have each subclass call super with the appropriate arguments.

We do have a good bit of duplication, but I think a utility function would be better for the average case as there are some really whacky plots.

adamjstewart
adamjstewart previously approved these changes Feb 26, 2022
@adamjstewart adamjstewart merged commit b2e178f into microsoft:main Mar 20, 2022
remtav pushed a commit to remtav/torchgeo that referenced this pull request May 26, 2022
* add own plot method and data.py

* clean up data.py

* version changed instead of added

* Update cbf.py

* Any instead of Tensor

* Fix VectorDataset tests

* Plot method in base class no longer needed/tested

* Removing unused imports

* Remove type ignore from openbuildings

* Fix tests

* Black formatting

Co-authored-by: Caleb Robinson <calebrob6@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* add own plot method and data.py

* clean up data.py

* version changed instead of added

* Update cbf.py

* Any instead of Tensor

* Fix VectorDataset tests

* Plot method in base class no longer needed/tested

* Removing unused imports

* Remove type ignore from openbuildings

* Fix tests

* Black formatting

Co-authored-by: Caleb Robinson <calebrob6@gmail.com>
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 testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants