make dimension.py xarray compatible #397
Conversation
There was a problem hiding this comment.
Fantastic work @mats-knmi ! Code is really looking much better (and this PR has a negative impact on the number of lines, brilliant!) and I know that the code you had to adapt is not easy to navigate... so well done!
I only had few minor comments, mostly concerning documentation - how are we going to make sure that the structure of the dataset is clearly documented? perhaps it could also be a good idea to add some basic input validator with a decorator, what do you think?
pysteps/utils/dimension.py
Outdated
| """Aggregate fields in time. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| R: array-like | ||
| Array of shape (t,m,n) or (l,t,m,n) containing | ||
| dataset: Dataset |
There was a problem hiding this comment.
I would still mention here that this is an xarray dataset, even if it's already obvious from the type hints
There was a problem hiding this comment.
also, the methods expect certain attributes, not just any kind of dataset. These should be mentioned somehow. See for example the description of "metadata" where we referenced the documention in pysteps.io.importers.
There was a problem hiding this comment.
Yes indeed a documentation similar to the "metadata" description (but for the dataset structure), which we can link to from these docstrings, would be very a very useful addition. I will take a look at this.
There was a problem hiding this comment.
| dataset: Dataset | |
| dataset: xarray.Dataset |
Is this what you mean?
pysteps/utils/dimension.py
Outdated
| """ | ||
| Upscale fields in space. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| R: array-like | ||
| Array of shape (m,n), (t,m,n) or (l,t,m,n) containing a single field or | ||
| dataset: Dataset |
There was a problem hiding this comment.
need to update the name in the rest of the docstrings (see the text for space_window)
| metadata: dict | ||
| The metadata with updated attributes. | ||
| dataset: Dataset | ||
| The new dataset. |
There was a problem hiding this comment.
Is it self-explanatory how the fields are aggregate with a given spatial window?
pysteps/utils/dimension.py
Outdated
|
|
||
| _aggregation_methods = dict( | ||
| sum=np.sum, mean=np.mean, nanmean=np.nanmean, nansum=np.nansum | ||
| ) | ||
|
|
||
|
|
||
| def aggregate_fields_time(R, metadata, time_window_min, ignore_nan=False): | ||
| def aggregate_fields_time(dataset: xr.Dataset, time_window_min, ignore_nan=False): |
There was a problem hiding this comment.
need to specify the type of the output, as it's not detailed in the docstrings
|
@dnerini I have added a ticket to the project backlog, for a basic input validator with a decorator. |
|
@dnerini I believe I have also taken care of the remaining comments you had, let me know what you think |
mats-knmi
force-pushed
the
xarray/convert-dimension
branch
from
ee4bbed
to
cb25820
Compare
* make dimension.py xarray compatible * convert final method in the dimension module * nanmin in stead of zerovalue in square domain method * make test steps skill run * undo accidental change * remove commented out code * The dataset can contain more than one dataarray * Address pull request comments * Add links to dataset documentation everywhere
* make dimension.py xarray compatible * convert final method in the dimension module * nanmin in stead of zerovalue in square domain method * make test steps skill run * undo accidental change * remove commented out code * The dataset can contain more than one dataarray * Address pull request comments * Add links to dataset documentation everywhere
Converted the methods in
dimension.pyto work with the new xarray datamodel. Since these methods can largely be replaced by xarray functionality, I updated these methods to make use of this.