-
Notifications
You must be signed in to change notification settings - Fork 59
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
QuantileDeltaMapping adjustment fails with seasonal grouping #1704
Comments
Digging deeper the problem seems to happen in sdba/utils.py |
I had a look at what happens for What should be the expected behaviour for seasons happy to implement that, I guess we could either not perform making it cyclic or invent some string? e.g. |
Hi @saschahofmann ! Sorry for the delay. Indeed, we might have completely overlooked the seasonal grouping when implementing the other parts. I'm not sure inventing strings will completely solve this. But I would still try that first, in But if it still doesn't work, may be we must abandon the idea of having strings as coordinates... If numbers are needed, I think the |
Would it make sense to replace the grouped coordinates in the beginning with numbers and finally return them to the array strings of seasons after the operations have been performed? Making it specific for the seasonal grouping wouldn't be very elegant but right now I can not come up with another case where this would happen? I think there is no grouping that e.g. returns weekday names? Alternatively, it could be something that's applied to all string groups? |
Yes it would make sense, but I'm not sure where that would take place ? In I don't think it makes much sense to support weekdays... Even then, these have a standard (pythonic) translation into integers, which we could use. As such, |
### Pull Request Checklist: - This PR fixes #1704 - [x] Added `test_seasonal` test to the QDM suit - [x] CHANGES.rst has been updated (with summary of main changes) - [x] Link to issue (:issue:`number`) and pull request (:pull:`number`) has been added ### What kind of change does this PR introduce? * In sdba/utils.py `interp_on_quantiles` seasons are now cast to integers to ensure we can add_cyclic_bounds. For this, I also had to overwrite the get_index function of the `Grouper` class. On a first class, I think that makes sense since in any case it doesn't make much sense to interp a string variable. ### Does this PR introduce a breaking change? Hopefully not.
Setup Information
Description
There seems to be a bug when trying to adjust a QuantileDeltaMapping with seasonal grouping. The training seems to work just fine.
See below for modified version of the example on the xclim docs
Steps To Reproduce
Create the data as in the official example
Trigger failing adjustment
Fails with this in xclim/sdba/base.py:703:
As mentioned above something like this passes just fine (monthly grouping)
Additional context
No response
Contribution
Code of Conduct
The text was updated successfully, but these errors were encountered: