Implement the Prism DEM datasets, add tests. #28
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Nice work @BSchilperoort . Very clean implementation. One small issue about the recipe. If you fix the time thingy for xarray (just like canopy-height dataset), then everything should work.
Tbh, at the beginning I thought the implementation of prism is the same as canopy and many things can be merged. But then when diving into the details, just notice that many things are actually different. Devils in the details 😂.
| # The prism DEM variable & coords already follow the CF/Zampy convention | ||
| for variable in ds.variables: | ||
| variable_name = str(variable) # Cast to string to please mypy. | ||
| if variable_name != "time": | ||
| ds[variable_name].attrs["units"] = str( | ||
| VARIABLE_REFERENCE_LOOKUP[variable_name].unit | ||
| ) | ||
| ds[variable_name].attrs["description"] = VARIABLE_REFERENCE_LOOKUP[ | ||
| variable_name | ||
| ].desc |
There was a problem hiding this comment.
This is duplicated:
zampy/src/zampy/datasets/eth_canopy_height.py
Lines 284 to 293 in 08a7ee4
Well....It is a very tiny part of the code. If you like, we can keep it for now (since snonarcloud doesn't cry 😂 ). Later when we have more datasets which again touches the same code, we can merge them, just for quality of life changes.
There was a problem hiding this comment.
Ah yeah, this does overlap. We can probably make it more generic and wrap it into a separate (util) function, however that might be best left for when we make the whole attribute writing more complete.
There was a problem hiding this comment.
That's also my thought. Great!
|
The API works nicely. But when I tried with the recipe, an error occur: ~/EcoExtreML/temp via 🐍 v3.10.12 (ecoextreml) took 2s
❯ zampy prism90_dem_recipe.yml
Executing recipe: prism90_dem_recipe.yml
Copernicus_DSM_30_N50_00_E003_00.tar: 8.97MB [00:01, 5.29MB/s]
elevation renamed to elevation.
Conversion of dataset 'prism-dem-90' following ALMA convention is complete!
Traceback (most recent call last):
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/dataset.py", line 1382, in _construct_dataarray
variable = self._variables[name]
KeyError: 'time'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/yangliu/venv/ecoextreml/bin/zampy", line 8, in <module>
sys.exit(run_recipe())
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
File "/home/yangliu/EcoExtreML/zampy/src/zampy/cli.py", line 13, in run_recipe
rm.run()
File "/home/yangliu/EcoExtreML/zampy/src/zampy/recipe.py", line 121, in run
ds = ds.resample(time=self.frequency).mean()
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/dataset.py", line 9322, in resample
return self._resample(
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/common.py", line 982, in _resample
dim_coord = self[dim]
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/dataset.py", line 1473, in __getitem__
return self._construct_dataarray(key)
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/dataset.py", line 1384, in _construct_dataarray
_, name, variable = _get_virtual_variable(self._variables, name, self.dims)
File "/home/yangliu/venv/ecoextreml/lib/python3.10/site-packages/xarray/core/dataset.py", line 196, in _get_virtual_variable
raise KeyError(key)
KeyError: 'time'Clearly it is due to the missing time axis, which is required in the recipe for saving the output after conversion. But it is easy to fix, just add a time coord, similar to canopy dataset. |
| da = da.sortby(["x", "y"]) # sort the dims ascending | ||
| da = da.isel(band=0) # get rid of band dim | ||
| da = da.drop_vars(["band", "spatial_ref"]) # drop unnecessary coords | ||
| ds = da.to_dataset() |
There was a problem hiding this comment.
Add something similar to canopy-height dataset to fix the recipe, for instance:
zampy/src/zampy/datasets/eth_canopy_height.py
Lines 270 to 272 in 08a7ee4
|
@geek-yang thanks for the review! I had fixed the missing time dim issue, but seem to have forgotten to commit it 🤦♂️. Instead of adding a time coordinate, I modify |
|
Kudos, SonarCloud Quality Gate passed! |
A demo notebook is included in demo/prism_dem_demo.ipynb