Skip to content

Hide numpy warnings for initial operations on unscaled quantity values - #5153

Merged
mhvk merged 6 commits into
astropy:masterfrom
adrn:units/hide-numpy-warning
Jul 9, 2016
Merged

mhvk merged 6 commits into
astropy:masterfrom
adrn:units/hide-numpy-warning

Conversation

@adrn

@adrn adrn commented Jul 3, 2016 •

Copy link
Copy Markdown
Member

This is an attempt to fix #3240.

Summarizing my understanding of the problem: numpy ufuncs will first operate directly on a quantity's value before it is scaled properly. This can cause a warning for things that are valid operations. For example, arcsin of the quantity 15 pc / kpc -- arcsin(15) will throw a warning, but after scaling, arcsin(0.015) is fine. This PR silences warnings like this by utilizing the warnings.catch_warnings context manager to silence numpy Runtime warnings in between __array_prepare__ and __array_wrap__. (I use the context manager to do this because it seems that temporarily catching a warning and then returning to the previous state is actually a nontrivial task, e.g., http://stackoverflow.com/questions/2390766/how-do-i-disable-and-then-re-enable-a-warning)

cc @mhvk

Closes #3240

@adrn

adrn commented Jul 3, 2016

Copy link
Copy Markdown
Member Author

Because of the hacky way I use the warnings.catch_warnings context manager, I think (as previously noted) a luckily timed ctrl+c could lead to strange behavior...but the worst that could happen is these warnings won't appear.

@adrn

adrn commented Jul 3, 2016

Copy link
Copy Markdown
Member Author

I should also say that an alternate to this is to add an issue to Known Issues and wait for numpy 1.12

@mhvk

mhvk commented Jul 4, 2016

Copy link
Copy Markdown
Contributor

This look OK, though I'm slightly wary about about adding execution time. Could you do a few quick timing checks (on single values or small arrays, where it would matter most, and for simple ufuncs like addition). If speed is an issue, we could make a specific list of ufuncs for which this problem might occur.

@mhvk

mhvk commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

A propos ensuring it only gets done when needed: the very simplest optimization is to turn on the warnings filter only inside the if any(converters): clause. But my sense is that we should be a little more conservative, and only do it for functions where it actually matters.

@adrn

adrn commented Jul 7, 2016

Copy link
Copy Markdown
Member Author

@mhvk For this type of warning, I think this can only happen in inverse trig functions: arcsin, arccos, arccosh, arctanh -- so how about:

if any(converters):
    ...

    if function in [np.arcsin, np.arccos, np.arcosh, np.arctanh]:
        # filter warning

@mhvk

mhvk commented Jul 7, 2016

Copy link
Copy Markdown
Contributor

Yes, I like the explicitness. In order to make it even faster it would be good to define the list of functions at the top as a set, i.e., something like

_UNIT_NOT_INITIALISED = "(Unit not initialised)"
_UFUNCS_FILTER_WARNINGS = {np.arcsin, np.arccos, np.arccosh, np.arctanh}

...
    if any(converters):
       ...
        if function in _UFUNCS_FILTER_WARNINGS:

@adrn
adrn force-pushed the units/hide-numpy-warning branch from c2bdeaf to 95bf14e Compare July 7, 2016 16:15
@adrn

adrn commented Jul 7, 2016

Copy link
Copy Markdown
Member Author

@mhvk OK, good idea!

Comment thread astropy/units/quantity.py Outdated
return result

def __array_wrap__(self, obj, context=None):
if hasattr(self, '_catch_warnings'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should have seen this before: just so the no-conversion case doesn't get slowed down, can you move this to l.504 (i.e., within the if hasattr(obj, '_converters') clause)...

@mhvk mhvk added this to the v1.2.2 milestone Jul 7, 2016
@mhvk

mhvk commented Jul 7, 2016

Copy link
Copy Markdown
Contributor

This looks good modulo the small movement requested above. Also, a changelog entry... I think this is properly classified as a bug, so it would be in the 1.2.2 section. And we might as well have a test case to ensure this doesn't come back: maybe simply the one from #3240 (maybe after test_arcsin_invalid_units in test_quantity_ufuncs.py).

@adrn

adrn commented Jul 8, 2016

Copy link
Copy Markdown
Member Author

@mhvk Thanks for the comments!

@mhvk mhvk self-assigned this Jul 9, 2016
@mhvk

mhvk commented Jul 9, 2016

Copy link
Copy Markdown
Contributor

Looks good, merging! Thanks, @adrn!

@mhvk
mhvk merged commit a6c13d9 into astropy:master Jul 9, 2016
eteq pushed a commit that referenced this pull request Dec 21, 2016
Hide numpy warnings for initial operations on unscaled quantity values
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.

units: warning from numpy operation on Quantity arithmetic

3 participants