Add an initial version of WMSDataset #1965

ianturton · 2024-03-27T14:12:11Z

Fixes #1963

ianturton · 2024-03-27T14:16:01Z

@microsoft-github-policy-service agree

adamjstewart

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

adamjstewart · 2024-03-27T17:02:43Z

requirements/required.txt

@@ -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


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.

Also need to add to requirements/min-reqs.old in order to get the minimum tests to pass.

tests/datasets/test_wms.py

adamjstewart · 2024-03-27T17:05:16Z

torchgeo/datasets/wms.py

@@ -0,0 +1,145 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+# Author: Ian Turton, Glasgow University ian.turton@gla.ac.uk


We don't normally edit the license header. Your contribution will still be included in the git history though.

adamjstewart · 2024-03-27T17:06:01Z

torchgeo/datasets/wms.py

+
+import torchvision.transforms as transforms
+from owslib.map.wms111 import ContentMetadata
+from owslib.wms import WebMapService


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.

@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.

adamjstewart · 2024-03-27T17:07:01Z

torchgeo/datasets/wms.py

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


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.

Also, can you add:

.. versionadded:: 0.6

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

adamjstewart · 2024-03-27T17:08:23Z

torchgeo/datasets/wms.py

+    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


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.

adamjstewart · 2024-03-27T17:09:20Z

torchgeo/datasets/wms.py

+        )
+        sample = {"crs": self.crs, "bbox": query}
+
+        transform = transforms.Compose([transforms.ToTensor()])


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

It's a JPEG or PNG image can I convert that directly to a tensor?

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 · 2024-03-27T17:34:14Z

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 · 2024-03-27T18:54:31Z

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.

tests/datasets/test_wms.py

adamjstewart · 2024-04-07T12:32:36Z

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

initial version of WMSDataset

92d2729

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

fix requirements

4e1ee53

ianturton and others added 4 commits March 27, 2024 14:16

Merge branch 'main' into wms

3dd0a04

fix formatting

d11231b

Merge branch 'wms' of github.com:ianturton/torchgeo into wms

1e819a6

fix black issues

b8698ac

adamjstewart added this to the 0.6.0 milestone Mar 27, 2024

isort

dc99c2b

ashnair1 changed the title ~~Add an initial version of WMSDataset #1963~~ Add an initial version of WMSDataset Mar 27, 2024

Ian Turton added 2 commits March 27, 2024 16:53

QA fixes

31fc997

add requests, might fix mypy issue?

7182c96

adamjstewart requested changes Mar 27, 2024

View reviewed changes

adamjstewart reviewed Mar 27, 2024

View reviewed changes

tests/datasets/test_wms.py Outdated Show resolved Hide resolved

ashnair1 reviewed Mar 27, 2024

View reviewed changes

tests/datasets/test_wms.py Outdated Show resolved Hide resolved

Ian Turton added 3 commits April 5, 2024 17:10

remove requests

2d9000c

remove requests

7752e95

remove online tag and check each test is online

95b49f1

Ian Turton added 2 commits April 8, 2024 11:53

update to 3.10 syntax

a8acd0a

make OWSlib more optional

2bf3c2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an initial version of WMSDataset #1965

Add an initial version of WMSDataset #1965

ianturton commented Mar 27, 2024 •

edited by ashnair1

ianturton commented Mar 27, 2024

adamjstewart left a comment

adamjstewart Mar 27, 2024

adamjstewart Apr 7, 2024

adamjstewart Mar 27, 2024

adamjstewart Mar 27, 2024

isaaccorley Apr 8, 2024

adamjstewart Mar 27, 2024

adamjstewart Mar 27, 2024

adamjstewart Mar 27, 2024

adamjstewart Mar 27, 2024

ianturton Apr 8, 2024

isaaccorley Apr 8, 2024

ianturton commented Mar 27, 2024

ashnair1 commented Mar 27, 2024 •

edited

adamjstewart commented Apr 7, 2024



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

Add an initial version of WMSDataset #1965

Are you sure you want to change the base?

Add an initial version of WMSDataset #1965

Conversation

ianturton commented Mar 27, 2024 • edited by ashnair1

ianturton commented Mar 27, 2024

adamjstewart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianturton commented Mar 27, 2024

ashnair1 commented Mar 27, 2024 • edited

adamjstewart commented Apr 7, 2024

ianturton commented Mar 27, 2024 •

edited by ashnair1

ashnair1 commented Mar 27, 2024 •

edited