Skip to content

doymax/min compatible with dask #2231

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
Merged
merged 15 commits into from
Aug 27, 2025
Merged

doymax/min compatible with dask #2231

merged 15 commits into from
Aug 27, 2025

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Aug 23, 2025 •
edited by Zeitsperre
Loading

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGELOG.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?

  • Use apply_ufunc when input DataArray is a daskarray

Does this PR introduce a breaking change?

No

Other information:

The main dimension ('time') must still be unchunked.

@github-actions github-actions bot added the CI Automation and Contiunous Integration label Aug 23, 2025
coxipi
coxipi commented Aug 25, 2025
View reviewed changes
Copy link
Contributor Author

@coxipi coxipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

coxipi
coxipi commented Aug 25, 2025
View reviewed changes
@coxipi coxipi requested a review from aulemahal August 25, 2025 15:03
aulemahal
aulemahal approved these changes Aug 25, 2025
View reviewed changes
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doymax and doymin do not support all-nan slices currently, because xarray does not support those on argmin/argmax, thus it is impossible I think for i to contain NaNs, and thus the astype should be safe.

Autrement dit : a problem for the future.

@github-actions github-actions bot added the approved Approved for additional tests label Aug 25, 2025
@coveralls
Copy link

Coverage Status

coverage: 92.175% (-0.03%) from 92.204%
when pulling a733c3c on fix-argmin-func
into ecacf95 on main.

@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Aug 26, 2025 •
edited
Loading

@coxipi does this close #2021? Update: Added that this closes #2107

@coxipi coxipi merged commit a5d1361 into main Aug 27, 2025
35 of 36 checks passed
@coxipi coxipi deleted the fix-argmin-func branch August 27, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aulemahal aulemahal aulemahal approved these changes

Assignees
No one assigned
Labels
approved Approved for additional tests CI Automation and Contiunous Integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

indices.generic.doymax crashes when given a dask array
4 participants
@coxipi @coveralls @Zeitsperre @aulemahal