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

VectorDataset: extract mint/maxt from filename #1814

Merged
merged 7 commits into from
Jan 19, 2024

Conversation

oddeirikigland
Copy link
Contributor

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Jan 19, 2024
@adamjstewart adamjstewart changed the title Fixing mint and maxt insert for VectorDataset from filename VectorDataset: extract mint/maxt from filename Jan 19, 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.

Can you copy https://github.com/microsoft/torchgeo/blob/v0.5.1/torchgeo/datasets/geo.py#L323-L339 from RasterDataset to VectorDataset? You can remove these lines: https://github.com/microsoft/torchgeo/blob/v0.5.1/torchgeo/datasets/geo.py#L329-L333

We'll also need to add tests for this. I can point you to where these would be, but let me know if you need help.

@adamjstewart adamjstewart added this to the 0.6.0 milestone Jan 19, 2024
@oddeirikigland
Copy link
Contributor Author

@oddeirikigland please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@oddeirikigland
Copy link
Contributor Author

Can you copy https://github.com/microsoft/torchgeo/blob/v0.5.1/torchgeo/datasets/geo.py#L323-L339 from RasterDataset to VectorDataset? You can remove these lines: https://github.com/microsoft/torchgeo/blob/v0.5.1/torchgeo/datasets/geo.py#L329-L333

We'll also need to add tests for this. I can point you to where these would be, but let me know if you need help.

Added the docstring. I'll need some guidance for the tests, where is the similar test for RasterDataset?

@adamjstewart
Copy link
Collaborator

For testing, you'll want to edit tests/data/vector/data.py such that the generated fake file includes a year in the filename. Then edit CustomVectorDataset in tests/data/test_geo.py to add a filename_regex and date_format to match.

@github-actions github-actions bot added the testing Continuous integration testing label Jan 19, 2024
@oddeirikigland
Copy link
Contributor Author

For testing, you'll want to edit tests/data/vector/data.py such that the generated fake file includes a year in the filename. Then edit CustomVectorDataset in tests/data/test_geo.py to add a filename_regex and date_format to match.

added test

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.

Looks pretty straightforward to me. Hopefully we'll get some builtin datasets that use this feature in the future. Not sure what kind of data your group is working with but would love to have it in TorchGeo if it's public. #1813 might actually use this feature too.

@adamjstewart adamjstewart merged commit 79cba04 into microsoft:main Jan 19, 2024
21 checks passed
@oddeirikigland
Copy link
Contributor Author

Looks pretty straightforward to me. Hopefully we'll get some builtin datasets that use this feature in the future. Not sure what kind of data your group is working with but would love to have it in TorchGeo if it's public. #1813 might actually use this feature too.

Perfect! Thank you for the help 😃 unfortunately, we cannot share the data we are working with. I see #1813 has tested the feature already so that can cover it 👌

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.

2 participants