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

Implement ma.where and ma.nonzero #9760

Merged
merged 2 commits into from
Jan 2, 2023
Merged

Implement ma.where and ma.nonzero #9760

merged 2 commits into from
Jan 2, 2023

Conversation

Holmgren825
Copy link
Contributor

Implements da.ma.where and da.ma.nonzero. Essentially follows the implementation of da.where.

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions github-actions bot added array documentation Improve or add to documentation labels Dec 14, 2022
@Holmgren825
Copy link
Contributor Author

Holmgren825 commented Dec 16, 2022 •
edited

Surprised to see that the new tests are failing on 3.8. It is the implementation of nonzero that is failing. But this should not have changed between the 3.8 and 3.10 environments. I think I have tracked it down to some difference in the results from calling compute on a tuple of two dask arrays. a difference in what happens when np.array is called on a tuple of dask collections.

@phobson
Copy link
Contributor

phobson commented Dec 16, 2022

@Holmgren825 thanks for the bug report the PR! I think this will be a use contribution to the library.

It looks like you're on the right track -- good work sleuthing different between 3.8 and 3.10 -- that's surprising to me. With the holidays coming up, responses to this might be slow, but feel free to ping me here if something needs specific attention.

@Holmgren825
Implements ma.where and ma.nonzero. Includes tests.
cf12850
@Holmgren825
Change testing of masked nonzero
a2192ec 
 This changes the test(s) of masked `nonzero` to more closely follow how
 `da.nonzero` is tested. It means that we assert each row in the
 resulting arrays sequentially, instead of the whole arrays at once. I assume
 this is done to get around that calling `np.array` on a tuple of
 dask collections in 3.8 does not result in the correct array.
@Holmgren825
Copy link
Contributor Author

Changed the new test of da.ma.nonzero to more closely follow the test of da.nonzero. With this, the new tests pass for 3.8 - 3.11. The two failing tests from 3.11 are not related to this PR.

@mrocklin
Copy link
Member

mrocklin commented Jan 2, 2023

The two failing tests from 3.11 are not related to this PR.

Yeah, this is reported up in #9793 . I suspect changes due to upstream. Folks are out on the holidays currently and so this hasn't been fixed. Hopefully it'll be resolved next week.

I've looked over things here. To the extent that I grok masked arrays this seems good to me. Thank you for your efforts here @Holmgren825 . Merging.

Also, I notice that this is your first code contribution to this repository. Welcome!

@mrocklin mrocklin merged commit f721de6 into dask:main Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
array documentation Improve or add to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mask preserving where e.g. da.ma.where
4 participants
@Holmgren825 @GPUtester @phobson @mrocklin