Skip to content

MAINT: Make assert_array_compare more generic. #11756

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

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

charris
Copy link
Member

@charris charris commented Aug 16, 2018

Use np.any instead of method to be a bit more robust against bad
subclassing of ndarray.

Closes #11743.

@@ -704,7 +706,7 @@ def func_assert_same_pos(x, y, func=isnan, hasval='nan'):
# compared usefully, and for which .all() yields masked.
x_id = func(x)
y_id = func(y)
if (x_id == y_id).all() != True:
if npany(x_id != y_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be overkill to mock up an ndarray subclass for which this succeeds but the previous version fails for a test of the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but didn't want to tie us into that behavior. Could add a comment.

Copy link
Contributor

@jeffyancey jeffyancey left a comment

Choose a reason for hiding this comment

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

Since we are removing the != True reference in the comment on line 704, shouldn't we update the comment?

@charris charris force-pushed the fix-testing-utils branch 2 times, most recently from 689d0a3 to 06eb262 Compare August 17, 2018 19:16
@charris
Copy link
Member Author

charris commented Aug 17, 2018

@tylerjereddy Added a comment and made the changes more conservative.

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I found a typo but everything is green and seems sensible beyond that

# Both the != True comparison here and the cast to bool_ at the end are
# done to deal with `masked`, which cannot be compared usefully, and
# for which np.all yields masked. The use of the function np.all is
# for backcompatility with ndarray subclasses that changed the return
Copy link
Contributor

Choose a reason for hiding this comment

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

typo -> compatibility

Use np.all instead of the *.all method to be a bit more robust against
bad subclasses of ndarray that may change the behavior of the method.

Closes numpy#11743.
@charris charris force-pushed the fix-testing-utils branch from 06eb262 to 78efe63 Compare August 17, 2018 22:47
@charris charris merged commit 18f338a into numpy:master Aug 17, 2018
@charris charris deleted the fix-testing-utils branch August 17, 2018 22:48
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 17, 2018
@charris charris removed this from the 1.15.1 release milestone Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants