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 an initial version of WMSDataset #1965

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

ianturton
Copy link

@ianturton ianturton commented Mar 27, 2024

Fixes #1963

@github-actions github-actions bot added datasets Geospatial or benchmark datasets testing Continuous integration testing dependencies Packaging and dependencies labels Mar 27, 2024
@ianturton
Copy link
Author

@microsoft-github-policy-service agree

@adamjstewart adamjstewart added this to the 0.6.0 milestone Mar 27, 2024
@ashnair1 ashnair1 changed the title Add an initial version of WMSDataset #1963 Add an initial version of WMSDataset Mar 27, 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.

This looks like a great start! I commented on some of the immediately obvious software-y things. There are still a lot of technical questions (what happens if I change the CRS or res, how does WMS handle that?) but I'll save those for later. Thanks again for the PR!

pyproject.toml Outdated Show resolved Hide resolved
@@ -9,6 +9,7 @@ lightly==1.5.2
lightning[pytorch-extra]==2.2.1
matplotlib==3.8.3
numpy==1.26.4
owsLib==0.30.0
Copy link
Collaborator

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, and requirements/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.

Copy link
Collaborator

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.

tests/datasets/test_wms.py Outdated Show resolved Hide resolved
@@ -0,0 +1,145 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
# Author: Ian Turton, Glasgow University [email protected]
Copy link
Collaborator

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.


import torchvision.transforms as transforms
from owslib.map.wms111 import ContentMetadata
from owslib.wms import WebMapService
Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ianturton you can check torchgeo.datasets.chabud as an example. See how we check if h5py is available in the constructor and then import h5py in the method that uses it.



class WMSDataset(GeoDataset):
"""Allow models to fetch images from a WMS (at a good resolution)."""
Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, can you add:

.. versionadded:: 0.6

to the docstring? This will document what version of TorchGeo is required to use this feature.

Comment on lines +103 to +109
def getlayer(self) -> ContentMetadata:
"""Return the selected layer object."""
return self._layer

def layers(self) -> list[str]:
"""Return a list of availiable layers."""
return self._layers
Copy link
Collaborator

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.

)
sample = {"crs": self.crs, "bbox": query}

transform = transforms.Compose([transforms.ToTensor()])
Copy link
Collaborator

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

Copy link
Author

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?

Copy link
Collaborator

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

@ianturton
Copy link
Author

I can't work out what's going on with mypy:

(torchgeo) ➜  torchgeo git:(wms) pwd                                                
/home/iturton/code/torchgeo
(torchgeo) ➜  torchgeo git:(wms) mypy tests/datasets/test_wms.py 
Success: no issues found in 1 source file
(torchgeo) ➜  torchgeo git:(wms) mypy torchgeo/datasets/wms.py 
Success: no issues found in 1 source file
(torchgeo) ➜  torchgeo git:(wms) pre-commit run --all-files
pyupgrade................................................................Passed
isort....................................................................Passed
black....................................................................Passed
flake8...................................................................Passed
pydocstyle...............................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

tests/datasets/test_wms.py:5: error: Library stubs not installed for "requests"  [import-untyped]
tests/datasets/test_wms.py:5: note: Hint: "python3 -m pip install types-requests"
tests/datasets/test_wms.py:5: note: (or run "mypy --install-types" to install all missing stub packages)
tests/datasets/test_wms.py:5: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 313 source files)

and yes types-requests is installed

@ashnair1
Copy link
Collaborator

ashnair1 commented Mar 27, 2024

pre-commit creates and uses a separate isolated environment while running hooks and not your existing environment. To solve, this, you just need to add types-requests>=0.1.0 to the additional_dependencies under mypy hook in the .pre-commit-config.yaml. The pre-commit environment will be recreated with the updated dependencies installed and the hooks will pass.

@adamjstewart
Copy link
Collaborator

We recently updated the repo to use Python 3.10+. Some of your code uses Python 3.9 syntax. You can fix this with pyupgrade: https://torchgeo.readthedocs.io/en/stable/user/contributing.html#linters

@adamjstewart
Copy link
Collaborator

@ianturton is this still a work in progress? We're preparing for a new release in the next couple weeks. Trying to merge or close as many remaining PRs as possible.

@adamjstewart adamjstewart modified the milestones: 0.6.0, 0.7.0 Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasets Geospatial or benchmark datasets dependencies Packaging and dependencies testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a WMS Dataset
4 participants