Added Support for retrieving Sea Ice Thickness #62
Conversation
There was a problem hiding this comment.
Code looks fine; the requested code changes are very minor.
The demo notebook ran without problem, but the descriptive text still related to the soil moisture notebook from which it had been adapted. It's hard to do change suggestions on notebooks so I updated the text in the notebook and pushed a new commit -- please roll back and/or adapt if you disagree with anything there.
| @@ -9,3 +9,5 @@ dependencies: | |||
| - python-dateutil >=2.8.1 | |||
| - xarray >=0.18.2 | |||
| - xcube >=0.9.0 | |||
| # for support of cftime with matplotlib (required to run sea ice thickness notebook) | |||
| - nc-time-axis >=1.4.1 | |||
There was a problem hiding this comment.
I don't like having an additional dependency here purely for a demo notebook, but I don't really like any of the alternatives I can think of either (rely on the user to install it manually, or add a line to install it in the notebook). Maybe we can keep it here but leave it out of the conda-forge build recipe...
There was a problem hiding this comment.
Absolutely. We can handle it like a dependency that is only required for testing.
test/test_sea_ice_thickness.py
Outdated
| if end_date is None: | ||
| self.assertIsNone(descriptor.time_range[1]) | ||
| else: | ||
| self.assertEquals(end_date, descriptor.time_range[1]) |
There was a problem hiding this comment.
| self.assertEquals(end_date, descriptor.time_range[1]) | |
| self.assertEqual(end_date, descriptor.time_range[1]) |
assertEquals has been deprecated in favour of assertEqual.
test/test_sea_ice_thickness.py
Outdated
| self.assertIsNone(descriptor.time_range[1]) | ||
| else: | ||
| self.assertEquals(end_date, descriptor.time_range[1]) | ||
| self.assertEquals({'sea_ice_thickness', 'quality_flag', 'status_flag', |
There was a problem hiding this comment.
| self.assertEquals({'sea_ice_thickness', 'quality_flag', 'status_flag', | |
| self.assertEqual({'sea_ice_thickness', 'quality_flag', 'status_flag', |
test/test_sea_ice_thickness.py
Outdated
|
|
||
| def testGetSupportedDataIds(self): | ||
| ids = self.sea_ice_handler.get_supported_data_ids() | ||
| self.assertEquals({_ENVISAT_DATA_ID, _CRYOSAT_2_DATA_ID}, set(ids)) |
There was a problem hiding this comment.
| self.assertEquals({_ENVISAT_DATA_ID, _CRYOSAT_2_DATA_ID}, set(ids)) | |
| self.assertEqual({_ENVISAT_DATA_ID, _CRYOSAT_2_DATA_ID}, set(ids)) |
| @@ -0,0 +1,355 @@ | |||
| # MIT License | |||
| # | |||
| # Copyright (c) 2020-2021 Brockmann Consult GmbH | |||
There was a problem hiding this comment.
| # Copyright (c) 2020-2021 Brockmann Consult GmbH | |
| # Copyright (c) 2020-2022 Brockmann Consult GmbH |
| 'checked. The ICDR is based on ' | ||
| 'observations from CryoSat-2 only (from April 2015 onward).'), |
There was a problem hiding this comment.
| 'checked. The ICDR is based on ' | |
| 'observations from CryoSat-2 only (from April 2015 onward).'), | |
| 'checked. The ICDR is based on observations ' | |
| 'from CryoSat-2 only (from April 2015 onward).'), |
Co-authored-by: Pontus Lurcock <pontus.lurcock@brockmann-consult.de>
Co-authored-by: Pontus Lurcock <pontus.lurcock@brockmann-consult.de>
|
Very good update of the notebook, Pont, thank you! I accepted all your changes and will merge now. |
Solves #61. There is also a notebook. The one problem with this commit is that the lat and lon variables are removed from the dataset during normalization, which shouldn't happen. It is, however, not affecting the data variables and a problem that needs to be addressed in xcube, so it is out of this PR's scope.