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

Adding test coverage for RasterDataset.plot #430

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Adding test coverage for RasterDataset.plot #430

merged 1 commit into from
Feb 24, 2022

Conversation

calebrob6
Copy link
Member

@calebrob6 calebrob6 commented Feb 23, 2022

Changing unrelated test data in #415 and #429 dropped test coverage for part of RasterDataset.plot. This PR adds a specific test for these lines.

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

Once all GeoDataset datasets have their own plot method, RasterDataset.plot and VectorDataset.plot won't have any coverage at all. I think we'll need a PR like this in the meantime to prevent coverage from dropping.

In the long run, we have a few options:

  1. Keep RasterDataset.plot and VectorDataset.plot but update the function signature to match
  2. Make RasterDataset.plot and VectorDataset.plot abstract methods
  3. Drop RasterDataset.plot and VectorDataset.plot

1 will result in dead code that isn't used by any dataset, so I don't think it's a great idea. 2 will allow us to enforce a consistent API for all plot methods, although it also means you can no longer instantiate a RasterDataset without first subclassing it and adding a plot method. We might have to go with 3.

@calebrob6
Copy link
Member Author

calebrob6 commented Feb 24, 2022 via email

@adamjstewart adamjstewart merged commit 0164104 into main Feb 24, 2022
@adamjstewart adamjstewart deleted the cmap_test branch February 24, 2022 15:57
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants