-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Xarray percentile - support non-dask input and percentiles in a new #248
Conversation
@kieranricardo This looks brilliant! Thanks so much for updating this - I'm testing it now and should have some feedback early this week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Kieran, thanks so much for this, and apologies for the delay in reviewing it! The revised function works amazingly quickly and the quantile dimension is exactly what I had in mind - will make it much more compatible with code that used to user the slower .quantile
methods. A few bits of feedback - very happy for some of these to be pushed back to future work depending on complexity!
- It's likely users will want to apply this function to data that doesn't necessarily have a time dimension (e.g. I currently have data that is labelled by "year"). From what I can tell the function does actually work for different dimension names - is it currently based on the first dimension in the list? If so, I think at minimum we should update the docstring to communicate this.
- Related to the 1. above: one of the most powerful features of
xarray
is being able to specify thedim
over which to apply functions, e.g.dim=['x', 'y']
ordim='time'
to reduce data spatially or temporally. It would be amazing if this function could support this kind of functionality rather than being hard-coded totime
, but if we think it's best to leave it as a specialised time dimension reducing function, it would be good to emphasise this in the docstring too and point users to the slower funcs if they want to do more complicated things. - The current
nodata
behavior could perhaps be improved if this value was read directly from thenodata
attribute on the inputxr.DataArray
data, e.g.ds.nbart_red.nodata
, with a fallback toNaN
or a custom supplied value if this doesn't exist (I think this is what otherodc.tools
funcs do). I can see that there's a requirement for the nodata value to be the smallest/largest/NaN
, so perhaps this might require a warning if this isn't the case. - Rename the
quantile
arg toq
for compatibility with the numpy and xarray implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to start using this func for some upcoming coastal work, so I think the best plan might be to merge this as-is, and I can raise the extra suggestions below as an issue for future work. Thanks again @kieranricardo!
@kieranricardo can you please rebase against develop rather than merge, and also cleanup commit history a bit, maybe merge some commits, or even all of them. |
@Kirill888 will a squash and merge be sufficient? |
Codecov Report
@@ Coverage Diff @@
## develop #248 +/- ##
===========================================
+ Coverage 58.61% 58.78% +0.16%
===========================================
Files 79 79
Lines 6643 6670 +27
===========================================
+ Hits 3894 3921 +27
Misses 2749 2749
Continue to review full report at Codecov.
|
@kieranricardo I don't know is there one commit worth of changes in here? You still need to pull in changes from develop too |
This PR does a couple of things and addresses #209:
xr_percentile
function toxr_quantile_bands
xr_quantile
that outputs the quantiles as a dimensionxr_quantiles
andxr_quantiles_bands
@robbibt is this what you had in mind? :)