From 37929816c7fb020dfec0c55128743d8da6aae5b5 Mon Sep 17 00:00:00 2001 From: Pontus Lurcock Date: Wed, 3 Jan 2024 11:58:02 +0100 Subject: [PATCH] Improve OGC Coverages scaling implementation Mainly in response to PR review - Rename CoverageScaling.scale property to "factor" - Add docstrings and comments to CoverageScaling class - Add some missing type hints - Add unit tests for the coverages.util module - Update docstring for controllers.dataset_to_image - Make ScalingTest.test_scale_factor more thorough --- test/webapi/ows/coverages/test_controllers.py | 1 + test/webapi/ows/coverages/test_scaling.py | 9 +-- test/webapi/ows/coverages/test_util.py | 18 +++++ xcube/webapi/ows/coverages/controllers.py | 3 +- xcube/webapi/ows/coverages/scaling.py | 70 +++++++++++++++++-- xcube/webapi/ows/coverages/util.py | 4 +- 6 files changed, 94 insertions(+), 11 deletions(-) create mode 100644 test/webapi/ows/coverages/test_util.py diff --git a/test/webapi/ows/coverages/test_controllers.py b/test/webapi/ows/coverages/test_controllers.py index cc8509f9b..35898056b 100644 --- a/test/webapi/ows/coverages/test_controllers.py +++ b/test/webapi/ows/coverages/test_controllers.py @@ -91,6 +91,7 @@ def test_get_coverage_data_geo_subset(self): def test_get_coverage_data_netcdf(self): crs = 'OGC:CRS84' + # Unscaled size is 400, 400 query = { 'bbox': ['1,51,2,52'], 'datetime': ['2017-01-24T00:00:00Z/2017-01-27T00:00:00Z'], diff --git a/test/webapi/ows/coverages/test_scaling.py b/test/webapi/ows/coverages/test_scaling.py index e8dbd4edf..115ea59d6 100644 --- a/test/webapi/ows/coverages/test_scaling.py +++ b/test/webapi/ows/coverages/test_scaling.py @@ -16,7 +16,7 @@ def setUp(self): def test_default_scaling(self): scaling = CoverageScaling(CoverageRequest({}), self.epsg4326, self.ds) - self.assertEqual((1, 1), scaling.scale) + self.assertEqual((1, 1), scaling.factor) def test_no_data(self): with self.assertRaises(ApiError.NotFound): @@ -42,7 +42,8 @@ def test_scale_factor(self): scaling = CoverageScaling( CoverageRequest({'scale-factor': ['2']}), self.epsg4326, self.ds ) - self.assertEqual((2, 2), scaling.scale) + self.assertEqual((2, 2), scaling.factor) + self.assertEqual((180, 90), scaling.size) def test_scale_axes(self): scaling = CoverageScaling( @@ -50,7 +51,7 @@ def test_scale_axes(self): self.epsg4326, self.ds, ) - self.assertEqual((1.2, 3), scaling.scale) + self.assertEqual((1.2, 3), scaling.factor) self.assertEqual((300, 60), scaling.size) def test_scale_size(self): @@ -60,7 +61,7 @@ def test_scale_size(self): self.ds, ) self.assertEqual((240, 90), scaling.size) - self.assertEqual((1.5, 2), scaling.scale) + self.assertEqual((1.5, 2), scaling.factor) def test_apply_identity_scaling(self): # noinspection PyTypeChecker diff --git a/test/webapi/ows/coverages/test_util.py b/test/webapi/ows/coverages/test_util.py new file mode 100644 index 000000000..260431171 --- /dev/null +++ b/test/webapi/ows/coverages/test_util.py @@ -0,0 +1,18 @@ +import unittest +import xcube.core.new +import xcube.webapi.ows.coverages.util as util + + +class UtilTest(unittest.TestCase): + + def setUp(self): + self.ds_latlon = xcube.core.new.new_cube() + self.ds_xy = xcube.core.new.new_cube(x_name='x', y_name='y') + + def test_get_h_dim(self): + self.assertEqual('lon', util.get_h_dim(self.ds_latlon)) + self.assertEqual('x', util.get_h_dim(self.ds_xy)) + + def test_get_v_dim(self): + self.assertEqual('lat', util.get_v_dim(self.ds_latlon)) + self.assertEqual('y', util.get_v_dim(self.ds_xy)) diff --git a/xcube/webapi/ows/coverages/controllers.py b/xcube/webapi/ows/coverages/controllers.py index 44f7e4928..940ba9bbd 100644 --- a/xcube/webapi/ows/coverages/controllers.py +++ b/xcube/webapi/ows/coverages/controllers.py @@ -154,7 +154,7 @@ def get_coverage_data( # and CRS transformation, to make sure that the final size is correct. scaling = CoverageScaling(request, final_crs, ds) _assert_coverage_size_ok(scaling) - if scaling.scale != (1, 1): + if scaling.factor != (1, 1): cropped_gm = GridMapping.from_dataset(ds, crs=final_crs) scaled_gm = scaling.apply(cropped_gm) ds = resample_in_space(ds, source_gm=cropped_gm, target_gm=scaled_gm) @@ -433,6 +433,7 @@ def dataset_to_image( :param ds: a dataset :param image_format: image format to generate ("png" or "tiff") + :param crs: CRS of the dataset :return: TIFF-formatted bytes representing the dataset """ diff --git a/xcube/webapi/ows/coverages/scaling.py b/xcube/webapi/ows/coverages/scaling.py index f679c6c13..d041fa3c9 100644 --- a/xcube/webapi/ows/coverages/scaling.py +++ b/xcube/webapi/ows/coverages/scaling.py @@ -30,6 +30,13 @@ class CoverageScaling: + """Representation of a scaling applied to an OGC coverage + + This class represents the scaling specified in an OGC coverage request. + It is instantiated using a `CoverageRequest` instance, and can apply + itself to a `GridMapping` instance. + """ + _scale: Optional[tuple[float, float]] = None _final_size: Optional[tuple[int, int]] = None _initial_size: tuple[int, int] @@ -40,6 +47,15 @@ class CoverageScaling: def __init__( self, request: CoverageRequest, crs: pyproj.CRS, ds: xr.Dataset ): + """Create a new scaling from a coverages request object + + :param request: a request optionally including scaling parameters. + If any scaling parameters are present, the returned instance will + correspond to them. If no scaling parameters are present, a + default scaling will be used (currently 1:1) + :param crs: the CRS of the dataset to be scaled + :param ds: the dataset to be scaled + """ h_dim = get_h_dim(ds) v_dim = get_v_dim(ds) for d in h_dim, v_dim: @@ -82,7 +98,21 @@ def __init__( self._scale = (1, 1) @property - def scale(self) -> tuple[float, float]: + def factor(self) -> tuple[float, float]: + """Return the two-dimensional scale factor of this scaling + + The components of the scale tuple are expressed as downscaling + factors: values greater than 1 imply that the rescaled size + of the coverage in the corresponding dimension will be smaller than + the original size, and vice versa. + + If the scaling was initially specified as a final size rather than + a factor, the factor property is an estimate based on the dataset + dimensions; the effective factor may be different when the scaling + is applied to a GridMapping. + + :return: a 2-tuple of the x and y scale factors, in that order + """ if self._scale is not None: return self._scale else: @@ -92,6 +122,11 @@ def scale(self) -> tuple[float, float]: @property def size(self) -> tuple[float, float]: + """Return the final coverage size produced by this scaling + + :return: a 2-tuple of the scaled x and y sizes, in that order, + in pixels + """ if self._final_size is not None: return self._final_size else: @@ -110,7 +145,19 @@ def _get_xy_values( y = axis_to_value[axis] return x, y - def get_axis_from_crs(self, valid_identifiers: set[str]): + def get_axis_from_crs(self, valid_identifiers: set[str]) -> Optional[str]: + """Find an axis abbreviation via a set of possible axis identifiers + + This method operates on the CRS with which this scaling was + instantiated. It returns the abbreviation of the first axis in + the CRS which has either a name or abbreviation matching any string + in the supplied set. + + :param valid_identifiers: a set of axis identifiers + :return: the abbreviation of the first axis in this scaling's CRS + whose name or abbreviation is in the supplied set, or `None` if + no such axis exists + """ for axis in self._crs.axis_info: if not hasattr(axis, 'abbrev'): continue @@ -124,12 +171,27 @@ def get_axis_from_crs(self, valid_identifiers: set[str]): return axis.abbrev return None - def apply(self, gm: GridMapping): - if self.scale == (1, 1): + def apply(self, gm: GridMapping) -> GridMapping: + """Apply this scaling to a grid mapping + + The supplied grid mapping is regularized before being scaled. + + :param gm: a grid mapping to be scaled + :return: the supplied grid mapping, scaled according to this scaling. + If this scaling is 1:1, the returned grid mapping may be the + original object. + """ + if self.factor == (1, 1): return gm else: regular = gm.to_regular() source = regular.size + # Even if the scaling was specified as a factor, we calculate + # from the (inferred) final size. If a factor was given, + # self.size is the final size as calculated from the originally + # specified dataset, which is what the client would expect. The + # regularized GridMapping might have a different size, + # so we don't want to apply a specified factor to it directly. return regular.scale( (self.size[0] / source[0], self.size[1] / source[1]) ) diff --git a/xcube/webapi/ows/coverages/util.py b/xcube/webapi/ows/coverages/util.py index 0c23ee3a4..72e41316c 100644 --- a/xcube/webapi/ows/coverages/util.py +++ b/xcube/webapi/ows/coverages/util.py @@ -23,13 +23,13 @@ import xarray as xr -def get_h_dim(ds: xr.Dataset): +def get_h_dim(ds: xr.Dataset) -> str: return [ d for d in list(map(str, ds.dims)) if d[:3].lower() in {'x', 'lon'} ][0] -def get_v_dim(ds: xr.Dataset): +def get_v_dim(ds: xr.Dataset) -> str: return [ d for d in list(map(str, ds.dims)) if d[:3].lower() in {'y', 'lat'} ][0]