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.

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

fix(mpiarray): crash testing for an Ellipsis when given an ndarray index #200

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

jrs65
Copy link
Contributor

@jrs65 jrs65 commented Jun 17, 2022

No description provided.

@jrs65 jrs65 requested a review from tristpinsm June 17, 2022 16:40
Copy link
Contributor

@tristpinsm tristpinsm left a comment

Choose a reason for hiding this comment

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

These changes make sense, but as far as I can tell the new test and the changes to the ellipsis handling are not related. Should they be split into two commits?

@jrs65
Copy link
Contributor Author

jrs65 commented Jun 20, 2022

Yeah, they are related but it's a bit subtle. The problem with the old code was that if you pass in a numpy array as an array index it does an element wise comparison to Ellipsis and then complains that you need to use .all() or .any() on the result. So this test now checks that case and thus throws an error with the previous implementation.

@ljgray
Copy link
Contributor

ljgray commented Jun 28, 2022

Any reason this hasn't been merged? Ran into the Ellipsis crash issue so having to use this branch to get around it.

@jrs65 jrs65 merged commit 501c2c9 into master Jun 29, 2022
@jrs65 jrs65 deleted the mpiarray-ellipsis-handling branch June 29, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants