Skip to content

Quantity behaviour for np.isclose, np.allclose; some others #9463

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
mhvk merged 5 commits into from
Oct 26, 2019
Merged

Quantity behaviour for np.isclose, np.allclose; some others #9463

mhvk merged 5 commits into from
Oct 26, 2019

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Oct 25, 2019

Following on #9462, this changes the Quantity implementation for isclose and allclose to assume a default unit of the first argument for atol (including for its default of 1e-8). It sticks with the docstring behaviour for np.broadcast_* to avoid breaking existing code. It also explicitly disallows np.any, np.all (which is most logical but changes the error raised), and makes array_repr, array_str and array2string work (former two slightly hacky). All the latter failed for all but dimensionless arrays.

fixes #9462

@mhvk mhvk added units Bug Affects-dev PRs and issues that do not impact an existing Astropy release Enhancement labels Oct 25, 2019
@mhvk mhvk added this to the v4.0 milestone Oct 25, 2019
@mhvk mhvk mentioned this pull request Oct 25, 2019
Closed
11 tasks
@pllim
Copy link
Member

pllim commented Oct 25, 2019

Failures seem real.

@bsipocz
Copy link
Member

bsipocz commented Oct 25, 2019

Now this needs a rebase.

@mhvk mhvk force-pushed the quantity-np-isclose-allclose branch 2 times, most recently from eb84aa1 to 61a661a Compare October 25, 2019 18:01
@mhvk
Copy link
Contributor Author

mhvk commented Oct 25, 2019

OK, rebased and (hopefully) fixed.

bsipocz
bsipocz approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

I trust your tests a lot, and all looks reasonable, but I don't trust myself to know enough of the numpy internals to spot any serious issues.
So use my approval with a pinch of salt.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 25, 2019

Thanks for catching that copy & paste error!

@bsipocz
Copy link
Member

bsipocz commented Oct 25, 2019

I cancel CIs as there was a doctest issue in the previous run that still needs addressing:

https://travis-ci.org/astropy/astropy/builds/602941330?utm_source=github_status&utm_medium=notification

_______________________ [doctest] docs/known_issues.rst ________________________

154 

155 .. math::

156 

157     |a - b| \le (a_\textrm{tol} + r_\textrm{tol} \times |b|)

158 

159 This will result in the following traceback when using this with Quantities::

160 

161     >>> from astropy import units as u, constants as const

162     >>> import numpy as np

163     >>> np.isclose(500 * u.km/u.s, 300 * u.km / u.s)  # doctest: +IGNORE_EXCEPTION_DETAIL

Expected:

    Traceback (most recent call last):

    ...

    UnitConversionError: Can only apply 'add' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)

Got:

    False

/tmp/astropy-test-ww9rbgjk/lib/python3.7/site-packages/docs/known_issues.rst:163: DocTestFailure

@mhvk
Copy link
Contributor Author

mhvk commented Oct 25, 2019

Thanks, @bsipocz, goes to show that skipping the overall test run for lack of time comes back to bite one...

@bsipocz
Copy link
Member

bsipocz commented Oct 25, 2019

No worries, this is actually pretty good sign that you've got rid of yet another know issue :)

mhvk added 5 commits October 25, 2019 18:29
@mhvk
Assume default unit of first argument for atol in isclose, allclose.
b49ef2d 
Also explicitly not support np.any, np.all, etc.  This changes
exception from NotImplementedError to TypeError, arguably better.
@mhvk
Also make array_repr, and array_str work.
1640c8d 
Both failed on Quantity with units, so no risk of working code breaking.
@mhvk
Explicitly note that subok remains needed for broadcast_*
d284984
@mhvk
Also make array2string work.
31b4209 
Here, we cast ourselves as ndarray if no relevant formatter is given,
since we are doomed otherwise (and this is most consistent with how
other subclasses are treated). However, if a relevant formatter is given,
we just use it.
@mhvk
Adjust docs to note isclose now works well with numpy >=1.17.
b686adf
@mhvk mhvk force-pushed the quantity-np-isclose-allclose branch from 2ea8ea4 to b686adf Compare October 25, 2019 22:30
@mhvk
Copy link
Contributor Author

mhvk commented Oct 25, 2019

OK, rebased. Tests pass locally so keeping fingers crossed.

astrofrog
astrofrog approved these changes Oct 26, 2019
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@mhvk mhvk merged commit a169f1a into astropy:master Oct 26, 2019
@mhvk mhvk deleted the quantity-np-isclose-allclose branch October 26, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astrofrog astrofrog astrofrog approved these changes

@bsipocz bsipocz bsipocz approved these changes

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug Enhancement units

Projects

None yet

Milestone

v4.0

Development

Successfully merging this pull request may close these issues.

Decision for some __array_function__ overrides

4 participants

@mhvk @pllim @bsipocz @astrofrog