Merge branch 'master' into forman-617-levels_on_s3

forman · web-flow · commit 37785bca60a1 · 2022-03-01T09:31:48.000+01:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -10,8 +10,21 @@
 
 ### Fixes
 
+* Fixed `FsDataAccessor.write_data()` implementations, 
+  which now always return the passed in `data_id`. (#623)
+
+* Fixes an issue where some datasets seemed to be shifted in the 
+  y-(latitude-) direction and were misplaced on maps whose tiles 
+  are served by `xcube serve`. Images with ascending y-values are 
+  now tiled correctly. (#626)
+
 ### Other
 
+* Replace the dependency on the rfc3339-validator PyPI package with a
+  dependency on its recently created conda-forge package.
+
+* Remove unneeded dependency on the no longer used strict-rfc3339 package.
+
 ## Changes in 0.10.1
 
 ### Fixes
diff --git a/environment.yml b/environment.yml
@@ -31,11 +31,11 @@ dependencies:
   - rasterio >=1.2
   - requests >=2.25
   - requests-oauthlib >=1.3
+  - rfc3339-validator >=0.1.4  # for python-jsonschema date-time format validation
   - s3fs >=2021.6
   - scipy >=1.6.0
   - setuptools >=41.0
   - shapely >=1.6
-  - strict-rfc3339 >=0.7  # for python-jsonschema date-time format validation
   - tornado >=6.0
   - urllib3 >=1.26
   - xarray >=0.19
@@ -46,6 +46,3 @@ dependencies:
   - pytest >=4.4
   - pytest-cov >=2.6
   - requests-mock >=1.8.0
-  - pip
-  - pip:
-    - rfc3339-validator
diff --git a/test/core/store/fs/test_registry.py b/test/core/store/fs/test_registry.py
@@ -169,7 +169,9 @@ def assertDatasetSupported(
         self.assertNotIn(data_id, set(data_store.get_data_ids()))
 
         data = self.new_cube_data()
-        data_store.write_data(data, data_id, **write_params)
+        written_data_id = data_store.write_data(data, data_id, **write_params)
+        self.assertEqual(data_id, written_data_id)
+
         self.assertEqual({expected_data_type_alias},
                          set(data_store.get_data_types_for_data(data_id)))
         self.assertEqual(True, data_store.has_data(data_id))
diff --git a/test/util/test_tiledimage.py b/test/util/test_tiledimage.py
@@ -3,11 +3,11 @@
 import numpy as np
 import PIL.Image
 
-from xcube.util.tiledimage import ArrayImage
+from xcube.util.tiledimage import SourceArrayImage
 from xcube.util.tiledimage import ColorMappedRgbaImage
 from xcube.util.tiledimage import DirectRgbaImage
 from xcube.util.tiledimage import OpImage
-from xcube.util.tiledimage import TransformArrayImage
+from xcube.util.tiledimage import NormalizeArrayImage
 from xcube.util.tiledimage import trim_tile
 
 
@@ -26,7 +26,7 @@ def compute_tile(self, tile_x, tile_y, rectangle):
 class ColorMappedRgbaImageTest(TestCase):
     def test_default(self):
         a = np.linspace(0, 255, 24, dtype=np.int32).reshape((4, 6))
-        source_image = ArrayImage(a, (2, 2))
+        source_image = SourceArrayImage(a, (2, 2))
         cm_rgb_image = ColorMappedRgbaImage(source_image, format='PNG')
         self.assertEqual(cm_rgb_image.size, (6, 4))
         self.assertEqual(cm_rgb_image.tile_size, (2, 2))
@@ -41,9 +41,9 @@ def test_default(self):
         b = np.linspace(255, 0, 24, dtype=np.int32).reshape((4, 6))
         c = np.linspace(50, 200, 24, dtype=np.int32).reshape((4, 6))
         source_images = [
-            ArrayImage(a, (2, 2)),
-            ArrayImage(b, (2, 2)),
-            ArrayImage(c, (2, 2)),
+            SourceArrayImage(a, (2, 2)),
+            SourceArrayImage(b, (2, 2)),
+            SourceArrayImage(c, (2, 2)),
         ]
         cm_rgb_image = DirectRgbaImage(source_images, format='PNG')
         self.assertEqual(cm_rgb_image.size, (6, 4))
@@ -53,11 +53,11 @@ def test_default(self):
         self.assertIsInstance(tile, PIL.Image.Image)
 
 
-class TransformArrayImageTest(TestCase):
+class NormalizeArrayImageTest(TestCase):
     def test_default(self):
         a = np.arange(0, 24, dtype=np.int32).reshape((4, 6))
-        source_image = ArrayImage(a, (2, 2))
-        target_image = TransformArrayImage(source_image)
+        source_image = SourceArrayImage(a, (2, 2))
+        target_image = NormalizeArrayImage(source_image)
 
         self.assertEqual(target_image.size, (6, 4))
         self.assertEqual(target_image.tile_size, (2, 2))
@@ -77,37 +77,86 @@ def test_default(self):
         self.assertEqual(target_image.get_tile(2, 1).tolist(), [[16, 17],
                                                                 [22, 23]])
 
-    def test_flip_y(self):
-        a = np.arange(0, 24, dtype=np.int32).reshape((4, 6))
-        source_image = ArrayImage(a, (2, 2))
-        target_image = TransformArrayImage(source_image, flip_y=True)
+    def test_force_2d(self):
+        a = np.arange(0, 48, dtype=np.int32).reshape((2, 4, 6))
+        source_image = SourceArrayImage(a[0], (2, 2))
+        target_image = NormalizeArrayImage(source_image, force_2d=True)
 
         self.assertEqual(target_image.size, (6, 4))
         self.assertEqual(target_image.tile_size, (2, 2))
         self.assertEqual(target_image.num_tiles, (3, 2))
 
-        self.assertEqual(target_image.get_tile(0, 0).tolist(), [[18, 19],
-                                                                [12, 13]])
-        self.assertEqual(target_image.get_tile(1, 0).tolist(), [[20, 21],
-                                                                [14, 15]])
-        self.assertEqual(target_image.get_tile(2, 0).tolist(), [[22, 23],
-                                                                [16, 17]])
 
-        self.assertEqual(target_image.get_tile(0, 1).tolist(), [[6, 7],
-                                                                [0, 1]])
-        self.assertEqual(target_image.get_tile(1, 1).tolist(), [[8, 9],
-                                                                [2, 3]])
-        self.assertEqual(target_image.get_tile(2, 1).tolist(), [[10, 11],
-                                                                [4, 5]])
-
-    def test_force_2d(self):
-        a = np.arange(0, 48, dtype=np.int32).reshape((2, 4, 6))
-        source_image = ArrayImage(a[0], (2, 2))
-        target_image = TransformArrayImage(source_image, force_2d=True)
+class SourceArrayImageTest(TestCase):
 
-        self.assertEqual(target_image.size, (6, 4))
-        self.assertEqual(target_image.tile_size, (2, 2))
-        self.assertEqual(target_image.num_tiles, (3, 2))
+    def test_flip_y_tiled_evenly(self):
+        a = np.arange(0, 24, dtype=np.int32).reshape((4, 6))
+        source_array_image = SourceArrayImage(a, (2, 2), flip_y=True)
+
+        self.assertEqual(source_array_image.size, (6, 4))
+        self.assertEqual(source_array_image.tile_size, (2, 2))
+        self.assertEqual(source_array_image.num_tiles, (3, 2))
+
+        self.assertEqual(source_array_image.get_tile(0, 0).tolist(),
+                         [[18, 19],
+                          [12, 13]])
+        self.assertEqual(source_array_image.get_tile(1, 0).tolist(),
+                         [[20, 21],
+                          [14, 15]])
+        self.assertEqual(source_array_image.get_tile(2, 0).tolist(),
+                         [[22, 23],
+                          [16, 17]])
+
+        self.assertEqual(source_array_image.get_tile(0, 1).tolist(),
+                         [[6, 7],
+                          [0, 1]])
+        self.assertEqual(source_array_image.get_tile(1, 1).tolist(),
+                         [[8, 9],
+                          [2, 3]])
+        self.assertEqual(source_array_image.get_tile(2, 1).tolist(),
+                         [[10, 11],
+                          [4, 5]])
+
+    def test_flip_y_not_tiled_evenly(self):
+        a = np.arange(0, 30, dtype=np.int32).reshape((5, 6))
+        source_array_image = SourceArrayImage(a, (2, 2), flip_y=True)
+
+        self.assertEqual(source_array_image.size, (6, 5))
+        self.assertEqual(source_array_image.tile_size, (2, 2))
+        self.assertEqual(source_array_image.num_tiles, (3, 3))
+
+        self.assertEqual(source_array_image.get_tile(0, 0).tolist(),
+                         [[24, 25],
+                          [18, 19]])
+        self.assertEqual(source_array_image.get_tile(1, 0).tolist(),
+                         [[26, 27],
+                          [20, 21]])
+        self.assertEqual(source_array_image.get_tile(2, 0).tolist(),
+                         [[28, 29],
+                          [22, 23]])
+
+        self.assertEqual(source_array_image.get_tile(0, 1).tolist(),
+                         [[12, 13],
+                          [6, 7]])
+        self.assertEqual(source_array_image.get_tile(1, 1).tolist(),
+                         [[14, 15],
+                          [8, 9]])
+        self.assertEqual(source_array_image.get_tile(2, 1).tolist(),
+                         [[16, 17],
+                          [10, 11]])
+
+        tile_0_2 = source_array_image.get_tile(0, 2).tolist()
+        self.assertEqual(tile_0_2[0], [0, 1])
+        self.assertTrue(np.isnan(tile_0_2[1][0]))
+        self.assertTrue(np.isnan(tile_0_2[1][1]))
+        tile_1_2 = source_array_image.get_tile(1, 2).tolist()
+        self.assertEqual(tile_1_2[0], [2, 3])
+        self.assertTrue(tile_1_2[1][0])
+        self.assertTrue(tile_1_2[1][1])
+        tile_2_2 = source_array_image.get_tile(2, 2).tolist()
+        self.assertEqual(tile_2_2[0], [4, 5])
+        self.assertTrue(tile_2_2[1][0])
+        self.assertTrue(tile_2_2[1][1])
 
 
 class TrimTileTest(TestCase):
diff --git a/xcube/core/store/fs/impl/dataset.py b/xcube/core/store/fs/impl/dataset.py
@@ -195,7 +195,7 @@ def write_data(self,
                    data: xr.Dataset,
                    data_id: str,
                    replace=False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, xr.Dataset, name='data')
         assert_instance(data_id, str, name='data_id')
         fs, root, write_params = self.load_fs(write_params)
@@ -213,6 +213,7 @@ def write_data(self,
         except ValueError as e:
             raise DataStoreError(f'Failed to write'
                                  f' dataset {data_id!r}: {e}') from e
+        return data_id
 
     def delete_data(self,
                     data_id: str,
@@ -281,7 +282,7 @@ def write_data(self,
                    data: xr.Dataset,
                    data_id: str,
                    replace=False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, xr.Dataset, name='data')
         assert_instance(data_id, str, name='data_id')
         fs, root, write_params = self.load_fs(write_params)
@@ -302,3 +303,4 @@ def write_data(self,
         data.to_netcdf(file_path, engine=engine, **write_params)
         if not is_local:
             fs.put_file(file_path, data_id)
+        return data_id
diff --git a/xcube/core/store/fs/impl/geodataframe.py b/xcube/core/store/fs/impl/geodataframe.py
@@ -82,7 +82,7 @@ def get_write_data_params_schema(self) -> JsonObjectSchema:
     def write_data(self,
                    data: gpd.GeoDataFrame,
                    data_id: str,
-                   **write_params):
+                   **write_params) -> str:
         # TODO: implement me correctly,
         #  this is not valid for shapefile AND geojson
         assert_instance(data, (gpd.GeoDataFrame, pd.DataFrame), 'data')
@@ -95,6 +95,7 @@ def write_data(self,
         data.to_file(file_path, driver=self.get_driver_name(), **write_params)
         if not is_local:
             fs.put_file(file_path, data_id)
+        return data_id
 
 
 class GeoDataFrameShapefileFsDataAccessor(GeoDataFrameFsDataAccessor, ABC):
diff --git a/xcube/core/store/fs/impl/mldataset.py b/xcube/core/store/fs/impl/mldataset.py
@@ -18,6 +18,7 @@
 # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
+
 import os.path
 from typing import Dict, Any, List, Union, Tuple, Optional
 
@@ -80,7 +81,7 @@ def write_data(self,
                    data: Union[xr.Dataset, MultiLevelDataset],
                    data_id: str,
                    replace: bool = False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, (xr.Dataset, MultiLevelDataset), name='data')
         assert_instance(data_id, str, name='data_id')
         if isinstance(data, MultiLevelDataset):
@@ -100,7 +101,6 @@ def write_data(self,
 
         for index in range(ml_dataset.num_levels):
             level_dataset = ml_dataset.get_dataset(index)
-
             if base_dataset_id and index == 0:
                 # Write file "0.link" instead of copying
                 # level-0 dataset to "0.zarr"
@@ -128,6 +128,9 @@ def write_data(self,
                                                  consolidated=consolidated)
                     ml_dataset.set_dataset(index, level_dataset)
 
+        return data_id
+
+
 
 class FsMultiLevelDataset(LazyMultiLevelDataset):
     def __init__(self,
diff --git a/xcube/core/store/fs/store.py b/xcube/core/store/fs/store.py
@@ -34,13 +34,14 @@
 from xcube.util.assertions import assert_given
 from xcube.util.assertions import assert_in
 from xcube.util.assertions import assert_instance
+from xcube.util.assertions import assert_true
 from xcube.util.extension import Extension
 from xcube.util.jsonschema import JsonBooleanSchema
 from xcube.util.jsonschema import JsonIntegerSchema
 from xcube.util.jsonschema import JsonObjectSchema
 from xcube.util.jsonschema import JsonStringSchema
-from .accessor import STORAGE_OPTIONS_PARAM_NAME
 from .accessor import FsAccessor
+from .accessor import STORAGE_OPTIONS_PARAM_NAME
 from .helpers import is_local_fs
 from .helpers import normalize_path
 from ..accessor import DataOpener
@@ -299,12 +300,23 @@ def write_data(self,
         data_id = self._ensure_valid_data_id(writer_id, data_id=data_id)
         fs_path = self._convert_data_id_into_fs_path(data_id)
         self.fs.makedirs(self.root, exist_ok=True)
-        writer.write_data(data,
-                          fs_path,
-                          replace=replace,
-                          fs=self.fs,
-                          root=self.root,
-                          **write_params)
+        written_fs_path = writer.write_data(data,
+                                            fs_path,
+                                            replace=replace,
+                                            fs=self.fs,
+                                            root=self.root,
+                                            **write_params)
+        # Verify, accessors fulfill their write_data() contract
+        assert_true(fs_path == written_fs_path,
+                    message='FsDataAccessor implementations must '
+                            'return the data_id passed in.')
+        # Return original data_id (which is a relative path).
+        # Note: it would be cleaner to return written_fs_path
+        # here, but it is an absolute path.
+        # --> Possible solution: FsDataAccessor implementations
+        # should be responsible for resolving their data_id into a
+        # filesystem path by providing them both with fs and root
+        # arguments.
         return data_id
 
     def get_delete_data_params_schema(self, data_id: str = None) \
diff --git a/xcube/core/tile.py b/xcube/core/tile.py
diff --git a/xcube/util/jsonschema.py b/xcube/util/jsonschema.py
diff --git a/xcube/util/tiledimage.py b/xcube/util/tiledimage.py
