Workaround for some cases that end up in #347 #419
Conversation
| @@ -136,7 +134,7 @@ def test_normalize_inverted_lat(self): | |||
| chunks={'time': 1}) | |||
|
|
|||
| actual = normalize_dataset(ds) | |||
| xr.testing.assert_equal(actual, expected) | |||
| xr.testing.assert_equal(actual, ds) | |||
There was a problem hiding this comment.
The check of this test is now that latitude is not inverted anymore. One might argue that we don't need it anymore at all. At least, remove the lines were the 'expected' dataset is built and rename the test to 'test_lat_not_inverted'.
There was a problem hiding this comment.
I agree that the variable expected (lines 128-134) can be removed entirely, but I think it's still useful to keep the test itself as a definition of current behaviour, even if it's trivial. For the name I'd prefer test_normalize_on_increasing_latitudes, which just refers the input format rather than the expected behaviour, and means that there's one less place to update if/when we change it again. (I try to avoid/remove "inverted" where possible, because I find it ambiguous -- to me it just implies "the opposite of whatever we currently want in this context". "increasing" / "decreasing" are far clearer.)
xcube/core/normalize.py
Outdated
| # chunks in "lat" that have the smallest chunk first. This will | ||
| # fail when writing to Zarr. (Hack for #347) | ||
| # See also https://github.com/pydata/xarray/issues/2300 | ||
| # ds = _ensure_lat_decreasing(ds) |
There was a problem hiding this comment.
Why only commented out? Why not fully removed? And the whole method along with it?
There was a problem hiding this comment.
Maybe because it's only intended as a temporary hack? Not sure...
# Conflicts: # CHANGES.md
| @@ -136,7 +134,7 @@ def test_normalize_inverted_lat(self): | |||
| chunks={'time': 1}) | |||
|
|
|||
| actual = normalize_dataset(ds) | |||
| xr.testing.assert_equal(actual, expected) | |||
| xr.testing.assert_equal(actual, ds) | |||
There was a problem hiding this comment.
I agree that the variable expected (lines 128-134) can be removed entirely, but I think it's still useful to keep the test itself as a definition of current behaviour, even if it's trivial. For the name I'd prefer test_normalize_on_increasing_latitudes, which just refers the input format rather than the expected behaviour, and means that there's one less place to update if/when we change it again. (I try to avoid/remove "inverted" where possible, because I find it ambiguous -- to me it just implies "the opposite of whatever we currently want in this context". "increasing" / "decreasing" are far clearer.)
xcube/core/normalize.py
Outdated
| # chunks in "lat" that have the smallest chunk first. This will | ||
| # fail when writing to Zarr. (Hack for #347) | ||
| # See also https://github.com/pydata/xarray/issues/2300 | ||
| # ds = _ensure_lat_decreasing(ds) |
There was a problem hiding this comment.
Maybe because it's only intended as a temporary hack? Not sure...
latitude coordinates, as this creates datasets that are no longer writable
to Zarr. (addresses but not completely resolves Writing to zarr fails with message "specified zarr chunks would overlap multiple dask chunks" #347)
Checklist:
Add docstrings and API docs for any new/modified user-facing classes and functionsNew/modified features documented indocs/source/*docs/CHANGES.mdAssociated issues closed after merge