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
Tests for where overrides in __array_ufunc__ #14596
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
RTD failure is unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better coverage is always 👍 from me. Thanks!
I'll approve but give @byrdie a chance to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a few ideas for minor improvements
def test_exception_with_where_quantity(self): | ||
a = np.ones(2) | ||
where = np.ones(2, bool) << u.m | ||
with pytest.raises(TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the match=
keyword here?
with pytest.raises(TypeError): | |
with pytest.raises(TypeError, match=r"Cannot cast array data from .* to .* according to the rule .*"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea, though I get a different error on numpy-dev (which I'll use...):
TypeError: operand type(s) all returned NotImplemented from __array_ufunc__(<ufunc 'add'>, '__call__', array([1., 1.]), array([1., 1.]), out=(array([1., 1.]),), where=<Quantity [ True, True] m>): 'ndarray', 'ndarray', 'ndarray'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the different messages have anything in common that we can use some regex? If not, I guess we don't have to include "match".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I must've been using the wrong version of numpy since @mhvk's version is correct.
64d1a44
to
a1755a5
Compare
OK, adjusted. Will merge once tests pass. Thanks! |
This pull request is a short follow-up on #14590 to add (1) a test for
Quantity
that if one passes a quantity inwhere
, it will raise aTypeError
(in numpy >=1.25; before the unit is simply ignored); and (2) increase coverage fornp.fix
with anout
argument.cc @byrdie, @pllim