Fix map_blocks for DataArray with cftime and sdba_encode_cf #1674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 7, 2024
Co-authored-by: David Huard <[email protected]>
aulemahal
added a commit
to Ouranosinc/xscen
that referenced
this pull request
Mar 15, 2024
<!-- Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [] This PR addresses an already opened issue (for bug fixes /
features)
- This PR fixes #xyz
- [ ] (If applicable) Documentation has been added / updated (for bug
fixes / features).
- [ ] (If applicable) Tests have been added.
- [x] This PR does not seem to break the templates.
- [ ] CHANGES.rst has been updated (with summary of main changes).
- [ ] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added.
### What kind of change does this PR introduce?
Instead of setting the options only when calling xclim's `adjust`, we
set the options globally at import time.
This fixes the subtle issue noted in Ouranosinc/xclim#1674 : the context
options are not seen by all code when dask-backed data is used. as the
actual computation occurs outside of the context.
It also allows to change these options at run time.
`xc.set_options(sdba_encode_cf=False)` will fix #362, for example. This
was not possible earlier as the option context was within the function
and couldn't be modified by the user.
### Does this PR introduce a breaking change?
Yes, issue #362 will now occur on dask arrays too, until xclim's PR is
merged. But it can be circumvented as shown above.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduced
tl;dr : me fix issue.
I think this issue stems from a change in recent xarray, but the case was untested by xclim. The solution was to iterate over a list that is "disconnected" from the dictionary.
The issue occurs when a DataArray that has a coordinate made of
cftimeobjects is passed to amap_blocks-wrapped function and xclim's global optionsdba_encode_cfis True.The previous encoding code was iterating over the coordinates with
coords.items(). When it would modify a the cftime coord, this would fail as you can't modify a dictionary while it is being iterated upon.Only happens if the option to encode is activated, only happens if there is a cftime coord and only happens on
DataArrayobjects. I'm not 100% sure why, but I thinkDataset.coords.itemsdoes not map directly to a dict. We usually useDatasetobjects in function wrapped bymap_blocks(ormap_groups). The culprit here was the polynomial's trend computation which uses aDataArray.Reason for the apparent dask-dependency
The interesting part is that this bug does not depend on the fact that the data is using dask or not. But it led me to discover another "issue" or, rather, "edgecase" .
In
xscen.adjustwe wrap the xclim call with options:Here, for DQM, the
adjustmethod is wrapped bymap_blocks. Whensim_seluses dask, the actual adjustment is, of course, not executed here. Only when the computation is triggered will the adjustment function be run, and only then will it call the polynomial detrending. But at that moment, thesdba_encode_cfoption is not set anymore! So the issue was not raised for the "dask" case.When
sim_selwas not using dask, all the code was executed under the option statement and thus the issue was raised. Which is a bit dumb because the encoding is a micro-optimization made for dask, it's no use otherwise.Does this PR introduce a breaking change?
No