GeoDatasets should use separate values for x and y resolution #2601
Conversation
github-actions
bot
added
datasets
There was a problem hiding this comment.
Copilot reviewed 24 out of 38 changed files in this pull request and generated no comments.
Files not reviewed (14)
- torchgeo/datasets/eudem.py: Evaluated as low risk
- torchgeo/datasets/cms_mangrove_canopy.py: Evaluated as low risk
- torchgeo/datasets/astergdem.py: Evaluated as low risk
- torchgeo/datasets/enmap.py: Evaluated as low risk
- torchgeo/datasets/cdl.py: Evaluated as low risk
- torchgeo/datasets/agb_live_woody_density.py: Evaluated as low risk
- torchgeo/datasets/esri2020.py: Evaluated as low risk
- tests/datamodules/test_geo.py: Evaluated as low risk
- torchgeo/datasets/eurocrops.py: Evaluated as low risk
- tests/datasets/test_cbf.py: Evaluated as low risk
- tests/samplers/test_batch.py: Evaluated as low risk
- tests/samplers/test_single.py: Evaluated as low risk
- torchgeo/datasets/chesapeake.py: Evaluated as low risk
- tests/datasets/test_geo.py: Evaluated as low risk
torchgeo/datasets/geo.py
Outdated
| @@ -416,7 +416,7 @@ def __init__( | |||
| self, | |||
| paths: Path | Iterable[Path] = 'data', | |||
| crs: CRS | None = None, | |||
| res: float | None = None, | |||
| res: tuple[float, float] | None = None, | |||
There was a problem hiding this comment.
If possible, I would like all of these datasets to support both float and tuple[float, float] similar to how our Samplers work. We can use the same helper function to convert the former to the latter. This is also nice for backwards compatibility (which we have to start caring about soon).
There was a problem hiding this comment.
Also need to update docstrings to document the order of xres and yres.
There was a problem hiding this comment.
Which docstrings would you like this in?
There was a problem hiding this comment.
Every single docstring for every single GeoDataset with a res parameter.
|
The failing test is a bug present in our min version of rasterio (1.3.0.post1) in how it reads res from WarpedVRT. The minimum version that handles this correctly is 1.3.11.
|
| root = os.path.join('tests', 'data', 'raster', 'res_2-2_epsg_32631') | ||
| ds = RasterDataset(root, res=10.0) | ||
| assert ds.res == (10.0, 10.0) | ||
| ds.res = 20.0 # type: ignore[assignment] |
There was a problem hiding this comment.
As far as I can tell, mypy doesn't like it when your getter returns a single type but your setter accepts a union of types, maybe discussed here python/mypy#3004
|
The main ideas here are:
|
| @@ -139,7 +139,7 @@ def __init__( | |||
| self, | |||
| paths: Path | Iterable[Path] = 'data', | |||
| crs: CRS | None = None, | |||
| res: float | None = None, | |||
| res: tuple[float, float] | None = None, | |||
There was a problem hiding this comment.
All of these other datasets also need to be float | tuple[float, float] and need updated docstrings too.
This PR refactors GeoDatasets to use separate values to store the x and y resolutions. This is pretty important because not all raster pixels are square.
Some notes:
sizein (height, width) format for the samplersCloses #2594