Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Lazy indexing not lazy #1483

Closed
2 tasks done
aulemahal opened this issue Sep 27, 2023 · 0 comments · Fixed by #1484
Closed
2 tasks done

Lazy indexing not lazy #1483

aulemahal opened this issue Sep 27, 2023 · 0 comments · Fixed by #1484
Assignees
@aulemahal
Labels
bug Something isn't working
Milestone
v0.46.0

Comments

@aulemahal
Copy link
Collaborator

aulemahal commented Sep 27, 2023 •
edited
Loading

Setup Information

  • Xclim version: master
  • Python version: 3.11
  • Operating System: The best

Description

lazy_indexing triggers some computation when dask-backed auxiliary coordinates are present.

Steps To Reproduce

from dask.diagnostics import ProgressBar
ProgressBar().register()

import xclim as xc
from xclim import testing as xt

ds = xt.open_dataset('ERA5/daily_surface_cancities_1990-1993.nc')
dsc = ds.isel(location=0).chunk()

xc.indices.run_length.lazy_indexing(dsc.tas, dsc.tas.argmax('time'))

Additional context

This affects all indicators using lazy_indexing. The function is used by first_run, last_run, run_bounds, season (all members of xclim.indices.run_length).

I detected the issue with atmos.growing_season_end.

Contribution

  • I would be willing/able to open a Pull Request to address this bug.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal aulemahal added the bug Something isn't working label Sep 27, 2023
@aulemahal aulemahal added this to the v0.46.0 milestone Sep 27, 2023
@aulemahal aulemahal self-assigned this Sep 27, 2023
@aulemahal aulemahal mentioned this issue Sep 27, 2023
Merged
5 tasks
aulemahal added a commit that referenced this issue Oct 11, 2023
@aulemahal
Fix lazy indexing (#1484)
e53c125 
<!--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:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #1483
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [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?

* `lazy_indexing` will drop any auxiliary coords it find on the
intermediate array that was triggering computation. The output is not
affected.
* New `xclim.testing.helpers.assert_lazy`. A context manager to ensure
the code block is not triggering any computation. I might take time to
add it to other tests in another PR.
* ~Took the opportunity to fix a deprecation warning coming out of
`importlib.resources`. The way we opened and searched for module data
files was deprecated.~ Oups, I realized this deprecation concerns python
>= 3.9, but we still support python 3.8. This change doesn't justify
pinning a new python.

### Does this PR introduce a breaking change?
No.

### Other information:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aulemahal aulemahal

Labels
bug Something isn't working
Projects
None yet
Milestone
v0.46.0
Development

Successfully merging a pull request may close this issue.

Fix lazy indexing Ouranosinc/xclim
1 participant
@aulemahal