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

@@ -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'],

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,15 +42,16 @@ 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(
             CoverageRequest({'scale-axes': ['Lat(3),Lon(1.2)']}),
             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

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

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

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])
             )

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]