Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 an initial version of WMSDataset #1965
base: main
Are you sure you want to change the base?
Add an initial version of WMSDataset #1965
Changes from 8 commits
92d2729
4e1ee53
3dd0a04
d11231b
1e819a6
b8698ac
dc99c2b
31fc997
7182c96
2d9000c
7752e95
95b49f1
a8acd0a
2bf3c2a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this optional instead of required. So it should listed in
pyproject.toml
,requirements/datasets.txt
, andrequirements/min-reqs.old
. The lower bound will have to be tested. Easiest way is to pip-install older and older versions until the tests fail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to add to
requirements/min-reqs.old
in order to get the minimum tests to pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally edit the license header. Your contribution will still be included in the git history though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports should be done lazily inside of the class so that the rest of TorchGeo still works even if owslib isn't installed. If you grep for any of our other optional dataset dependencies, you should see example code for this you can copy-n-paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what "at a good resolution" means? Also would be good to explain what WMS is and why it is useful. For example, see the documentation for some of our other existing dataset base classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you add:
to the docstring? This will document what version of TorchGeo is required to use this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters/setters are generally considered to be bad practice in Python, especially if you don't actually need to do anything special. In this case, if we want the ability to access the attribute, let's just make it a public attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally use torchvision transforms. What is the output from wms, a numpy array? Let's just convert that directly to PyTorch without relying on PIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a JPEG or PNG image can I convert that directly to a tensor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T.ToTensor() also automatically divides by 255 which we should leave up to the user to decide. You can convert the image to a tensor using
torch.from_numpy(np.array(Image.open(path).astype(np.float32)))
. You can break this up over multiple lines so it's not too verbose