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

Moving function quantity_allclose #7235 #7252

Merged
merged 12 commits into from Apr 16, 2018
Merged

Moving function quantity_allclose #7235 #7252

merged 12 commits into from Apr 16, 2018

Conversation

rohanrajpal
Copy link
Contributor

Deprecated the quantity_allclose in astropy.tests.helper
Moved function quantity_allclose to astropy.units.quantity
Closes #7235

@astropy-bot
Copy link

astropy-bot bot commented Mar 2, 2018

Hi there @rohanrajpal 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@rohanrajpal rohanrajpal changed the title Moving function quantity_allclose Moving function quantity_allclose #7235 Mar 2, 2018
@pllim pllim added this to the v3.1 milestone Mar 2, 2018
pllim
pllim previously requested changes Mar 2, 2018
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Some comments below. Please also add a change log for 3.1 under "API changes" (see other entries for formatting).

@@ -465,7 +464,7 @@ def assert_quantity_allclose(actual, desired, rtol=1.e-7, atol=None,
rtol, atol),
**kwargs)


@deprecated("3.1.dev21533")
Copy link
Member

Choose a reason for hiding this comment

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

  • Just "3.1" is sufficient.
  • Please provide a string to explain where it has moved to.
  • To avoid code duplication, this function now should only have one functional line of code to just call the new one.

@@ -3,14 +3,13 @@
This module provides the tools used to internally run the astropy test suite
from the installed astropy. It makes use of the `pytest` testing framework.
"""

from ..utils.decorators import deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Please move local import to after import pytest.

@@ -8,6 +8,7 @@


# Standard library

Copy link
Member

Choose a reason for hiding this comment

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

Please remove extra line here.

@@ -107,7 +117,6 @@ def __next__(self):

next = __next__


Copy link
Member

Choose a reason for hiding this comment

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

There should be 2 lines between classes and functions. Please fix PEP8 warnings reported at https://travis-ci.org/astropy/astropy/jobs/348076730 (I just restarted the job because Travis has problem displaying the log).

This is a :class:`~astropy.units.Quantity`-aware version of
:func:`numpy.allclose`.
"""
return np.allclose(*_unquantify_allclose_arguments(a, b, rtol, atol),
Copy link
Member

Choose a reason for hiding this comment

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

@mhvk , should _unquantify_allclose_arguments be moved over too? (Since this is a "hidden" function, I don't think deprecation would be necessary if we decide to move it.)

Copy link
Member

Choose a reason for hiding this comment

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

Surely without moving that over astropy.units.quantity now has an import dependency on pytest?!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, _unquantity_allclose_arguments definitely needs to be moved here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhvk And the name _unquantity_allclose_arguments shall remain the same or changed like allclose ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name is fine as is -- and it is a private function anyway, so we can change it later if needed.

@@ -687,6 +696,7 @@ def _result_as_quantity(self, result, unit, out):
out._set_unit(unit)
return out


Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line.

@@ -115,7 +115,7 @@ def deprecated_func(*args, **kwargs):
category = AstropyDeprecationWarning

warnings.warn(message, category, stacklevel=2)

Copy link
Member

Choose a reason for hiding this comment

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

Please have your editor set to remove extraneous whitespace like this.

@rohanrajpal
Copy link
Contributor Author

@mwcraig

@@ -39,6 +39,15 @@
_UNIT_NOT_INITIALISED = "(Unit not initialised)"
_UFUNCS_FILTER_WARNINGS = {np.arcsin, np.arccos, np.arccosh, np.arctanh}

def quantity_allclose(a, b, rtol=1.e-5, atol=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I would vote for this just being called allclose as it's in units.quantity

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It should also be added to __all__, so that it is exposed as u.allclose().

Aside: needs an extra empty line above the function definition (pep8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After going through units.quantity all the attributes of all.All of them are classes which have functions inside them, so I have to do the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this would remain a function - it is just a coincidence that in this module only classes are exported.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

It seems we're slowly converging. What remains to be done:

  1. Change all occurrences of quantity_allclose in the code base (probably mostly in test files, but check).
  2. Look for a good place in docs/units to mention the function (not in "getting started", probably).

CHANGES.rst Outdated
@@ -61,6 +61,7 @@ astropy.table

astropy.tests
^^^^^^^^^^^^^
-Deprecated function ``quantity``allclose to helper
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting is wrong (see elsewhere in fine). Also, this is not very helpful to someone who is reading this. Write

The function ``quantity_allclose`` was moved to the ``units`` package with the new,
shorter name ``allclose``. It is deprecated in ``tests``. [#7252]

Copy link
Contributor

Choose a reason for hiding this comment

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

The comments about the changelog should still be addressed.

CHANGES.rst Outdated
@@ -71,7 +72,7 @@ astropy.time

astropy.units
^^^^^^^^^^^^^

-Added function ``quantity``allclose to quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, formatting is wrong (need space and empty line after, plus link to PR), and the description is too terse to be useful:

Added a units-aware ``allclose`` function (this was previously available in the
``tests`` module as ``quantity_allclose``) [#7252]

@@ -465,17 +464,16 @@ def assert_quantity_allclose(actual, desired, rtol=1.e-7, atol=None,
rtol, atol),
**kwargs)


@deprecated("3.1","moved function to astropy.units.quantity")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a helpful message. I think we can just use alternative instead of the message argument, as in alternative='astropy.units.allclose'

import numpy as np
return np.allclose(*_unquantify_allclose_arguments(a, b, rtol, atol),
**kwargs)
from ..units.quantity import quantity_allclose as quantity_allclose_units
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we seem to have reached agreement to call it allclose in units

@@ -39,6 +39,15 @@
_UNIT_NOT_INITIALISED = "(Unit not initialised)"
_UFUNCS_FILTER_WARNINGS = {np.arcsin, np.arccos, np.arccosh, np.arctanh}

def quantity_allclose(a, b, rtol=1.e-5, atol=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It should also be added to __all__, so that it is exposed as u.allclose().

Aside: needs an extra empty line above the function definition (pep8)

"""
Returns True if two arrays are element-wise equal within a tolerance.

This is a :class:`~astropy.units.Quantity`-aware version of
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are making this a much more visible function, it should have a proper docstring, defining the arguments, etc. See developer documentation (and other examples in code) for details on how this looks. This particular sentence would go under "Notes"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means _unquantify_allclose_arguments shall also have a proper docstring,right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It remains a private function and it is pretty obvious what it does, so I don't think it is essential, but feel free to add something short.

This is a :class:`~astropy.units.Quantity`-aware version of
:func:`numpy.allclose`.
"""
return np.allclose(*_unquantify_allclose_arguments(a, b, rtol, atol),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, _unquantity_allclose_arguments definitely needs to be moved here.

@rohanrajpal
Copy link
Contributor Author

Okay @mhvk
The changes you've requested have been done but the following remain
`

  1. Change all occurrences of quantity_allclose in the code base (probably mostly in test files, but check).
  2. Look for a good place in docs/units to mention the function (not in "getting started", probably).
    `
    I am manually trying to find the occurences but would appreciate if you could specify where to look, how to and which test files to modify.

@mhvk
Copy link
Contributor

mhvk commented Mar 11, 2018

@rohanrajpal - Just do the following:

find astropy -name \*.py -exec grep [^_]quantity_allclose {} \; -print

Note that it is fine to change not the tests themselves but just the import, as in:

from ...units import allclose as quantity_allclose

And the deprecation warnings will tell you whether you missed anything.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Thought I might as well review...


@deprecated("3.1",alternative='astropy.units._unquantify_allclose_arguments')
Copy link
Contributor

Choose a reason for hiding this comment

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

This one doesn't have to be deprecated - it is a private function. However, you do need to import it inside assert_quantity_allclose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but I do have to make a call to the new location of the function, in order to avoid code duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

That why I noted that you have to import it in assert_quantity_allclose

CHANGES.rst Outdated
@@ -61,6 +61,7 @@ astropy.table

astropy.tests
^^^^^^^^^^^^^
-Deprecated function ``quantity``allclose to helper
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments about the changelog should still be addressed.

@@ -30,7 +30,7 @@
check_output)

__all__ = ["Quantity", "SpecificTypeQuantity",
"QuantityInfoBase", "QuantityInfo"]
"QuantityInfoBase", "QuantityInfo","allclose"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after , - please follow pep8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I'll be careful now

@@ -40,6 +40,52 @@
_UFUNCS_FILTER_WARNINGS = {np.arcsin, np.arccos, np.arccosh, np.arctanh}


def allclose(a, b, rtol=1.e-5, atol=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a detail, but this function is not as important as the Quantity class - I would suggest moving it to the end of the file (with _unquantify_allclose_arguments)

@@ -841,7 +887,7 @@ def to(self, unit, equivalencies=[]):
--------
to_value : get the numerical value in a given unit.
"""
# We don't use `to_value` below since we always want to make a copy
# We don't use `to_value` below since we always want to make a Copy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? Just keep as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad have fixed this in the recent pull request
@mhvk can you review it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see a change here. Can you undo it?

@rohanrajpal
Copy link
Contributor Author

I've tried to make all the changes following pep8, I've noticed the pep8 error
helper.py:482:80: E501 line too long (87 > 79 characters)
in a lot of places, is this ignored while doing pep8 tests?

@mhvk
Copy link
Contributor

mhvk commented Mar 12, 2018

Indeed, long lines are ignored by default (though it is good to stick to it).

CHANGES.rst Outdated
- The function ``quantity_allclose`` was moved to the ``units`` package with the new,
shorter name ``allclose``. It is deprecated in ``tests``. [#7252]

- The function ``_unquantify_allclose_arguments`` was moved to the ``units`` package. It is deprecated in ``tests``. [#7252]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a private function, so we should not advertise it in any way (i.e., just remove this from the changelog).

CHANGES.rst Outdated
- Added a units-aware ``allclose`` function (this was previously available in the
``tests`` module as ``quantity_allclose``) [#7252]

- Added a units-aware ``_unquantify_allclose_arguments`` function (this was previously available in the
Copy link
Contributor

Choose a reason for hiding this comment

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

As above - remove this from the changelog.

@@ -5,7 +5,7 @@

import numpy as np

from ....tests.helper import quantity_allclose
from ....units.quantity import allclose as quantity_allclose
Copy link
Contributor

Choose a reason for hiding this comment

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

Total nitpick, but typically we import from as high a level as possible, so from ....units import ... (same for other files).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give a complete example? I'm getting a value error by importing with that method.Probably making a silly mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

from ....units import allclose as quantity_allclose

I hope this solves it; if not, please do debug yourself - this is taking me more time to answer than it should take you to solve.

@@ -17,11 +17,11 @@
from numpy.random import randn
from numpy import testing as npt

from ...tests.helper import (raises, quantity_allclose as allclose,
assert_quantity_allclose as assert_allclose)
from ...tests.helper import (raises, assert_quantity_allclose as assert_allclose)
Copy link
Contributor

Choose a reason for hiding this comment

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

parentheses can go now...

@@ -9,11 +9,10 @@
from ..builtin_frames import (ICRS, FK5, FK4, FK4NoETerms, Galactic,
Supergalactic, Galactocentric, HCRS, GCRS, LSR)
from .. import SkyCoord
from ...tests.helper import (quantity_allclose as allclose,
assert_quantity_allclose as assert_allclose)
from ...tests.helper import (assert_quantity_allclose as assert_allclose)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove parentheses

@@ -11,8 +11,9 @@
from .. import representation as r
from ..baseframe import frame_transform_graph
from ...tests.helper import (assert_quantity_allclose as assert_allclose,
quantity_allclose, catch_warnings)
catch_warnings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not aligned properly

import pytest
from ..utils.decorators import deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

As I'm nitpicking anyway, an open line before this statement (separating standard library from our own code).

Copy link
Member

Choose a reason for hiding this comment

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

Remove this since it isn't used.

return np.allclose(*_unquantify_allclose_arguments(a, b, rtol, atol),
**kwargs)
from ..units.quantity import allclose as quantity_allclose_units
quantity_allclose_units(a, b, rtol=1.e-5, atol=None, **kwargs)


def _unquantify_allclose_arguments(actual, desired, rtol, atol):
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be removed altogether. Nobody should be relying on this private routine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to remove the function quantity_allclose altogether?
Shouldn't a deprecated function call to the new location of the function?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, please look where the comment is made: you should remove _unquantity_allclose_arguments from utils -- it is a private function, so no deprecation is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You meant to remove from units not utils, right? If you do mean units, isn't that the place I moved the function _unquantity_allclose_arguments to?Also, this comment is made in tests/helper.py.Should I remove it from helper as well or keep it as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant utils - you moved _unquantity_allclose_arguments to units and there is no reason to keep it here, as you import it directly from units in assert_quantity_allclose.



def _unquantify_allclose_arguments(actual, desired, rtol, atol):
from .. import units as u
Copy link
Contributor

Choose a reason for hiding this comment

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

We are inside units here, so no need for this import; just change u.Quantity -> Quantity below (and same for UnitsError)

@mhvk
Copy link
Contributor

mhvk commented Mar 13, 2018

Quite a few nitpicks, but also some more substantial comments. While you are at it, could you also rebase & squash the commits. And of course please check the test failures (much easier if you run tests locally first to ensure things are OK).

@astrofrog
Copy link
Member

astrofrog commented Mar 19, 2018

I'm personally -1 on deprecating the old location (as in, I don't think we should raise a deprecation warning yet, but just silently allow it to work). This is a function that I've actually used in quite a few packages and I suspect others may have too, and it's one of these cases where it's not clear that deprecating it is actually necessary and is going to be a pain for affiliated package maintainers. I've been wanting for a while to write some kind of APE along these lines, but I don't think we always need to systematically emit deprecation warnings for old behavior (which then trigger CI failures) if keeping the old behavior isn't a burden (which it wouldn't be here). We should be striving for stability not purity, and this PR as-is is going to require affiliated package developers to have four lines (a try...except) to import this function instead of one now in order to support the current LTS and latest version.

TL;DR I think we should only deprecate things when they become a burden to maintain in astropy dev.

@mhvk
Copy link
Contributor

mhvk commented Mar 19, 2018

@astrofrog - OK, that makes sense to me as well. Just so we don't get another cycle

from ....units import allclose as quantity_allclose

and not define a deprecated quantity_allclose here.

@pllim
Copy link
Member

pllim commented Mar 19, 2018

@astrofrog , can't we still use it but just set pending=True? This way, it is still easy to grep to see if anything should be deprecated in the future. See https://github.com/astropy/astropy/blob/master/astropy/utils/decorators.py#L24

@rohanrajpal
Copy link
Contributor Author

rohanrajpal commented Mar 22, 2018

I'm sorry the recent pull request was made by mistake while trying to rebase and squash.
EDIT: All changes have been made, except for the one @pllim has suggested recently. Waiting for @astrofrog to reply to that suggestion.

@pllim
Copy link
Member

pllim commented Apr 14, 2018

@mhvk , I did a review and addressed your remaining concerns (missing doc and tests). Please have a look. If tests are passing now and you are satisfied, feel free to merge.

>>> u.allclose([1, 2] * u.m, [100, 200] * u.cm)
True
>>> u.isclose([1, 2] * u.m, [100, 20] * u.cm)
array([ True, False])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the annoying things that changed in recent numpy (and thus doesn't work across versions), that in numpy <=1.13, this has ", dtype=bool" appended; maybe just doctest: +SKIP...

This is because formatting is different across Numpy versions. [docs only]
@@ -30,7 +29,7 @@

__all__ = ['raises', 'enable_deprecations_as_exceptions', 'remote_data',
'treat_deprecations_as_exceptions', 'catch_warnings',
'assert_follows_unicode_guidelines', 'quantity_allclose',
Copy link
Member

@bsipocz bsipocz Apr 14, 2018

Choose a reason for hiding this comment

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

Can we do this on a bugfix branch? While importing with * is bad practice, this removal is an API change for those who do that.

@bsipocz
Copy link
Member

bsipocz commented Apr 16, 2018

As @astrofrog commented above (#7252 (review)), we need to remove this function for the __all__, but that needs to be done in a follow-up PR milestoned to 3.1, so the bugfix branch keeps working without issue.
Whoever merges, please also open that simple PR so it doesn't get forgotten.

@pllim
Copy link
Member

pllim commented Apr 16, 2018

@bsipocz and @drdavella , adding it back in __all__ breaks the Sphinx build in Travis. That is why I removed it in the first place. I am not comfortable merging this knowing Sphinx build would break.

@Cadair
Copy link
Member

Cadair commented Apr 16, 2018

You could always just use this as the master fix and then use my other PR as the backport. Completely removes any API change in the bugfix then.

@Cadair
Copy link
Member

Cadair commented Apr 16, 2018

(for @bsipocz we could merge that PR first and then revert the commit in this one) 😉

@bsipocz
Copy link
Member

bsipocz commented Apr 16, 2018

@pllim - Yes, that's a good point, and I'm not comfortable with breaking sphinx either (but somehow missed that it happened already).

Probably the simplest is to add it back either during the backport (either as part of this or a separate commit), or in a PR directly opened to 2.0.x once this one is backported.

np.testing.assert_allclose(*_unquantify_allclose_arguments(actual, desired,
rtol, atol),
**kwargs)
assert _quantity_allclose(actual, desired, rtol=rtol, atol=atol, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

@astrofrog , like this, correct?

@pllim
Copy link
Member

pllim commented Apr 16, 2018

🐴 💀

@pllim pllim merged commit 2d63a40 into astropy:master Apr 16, 2018
@pllim
Copy link
Member

pllim commented Apr 16, 2018

(I'll open a follow-up PR about that __all__ thingy. See #7381.)

@Cadair
Copy link
Member

Cadair commented Apr 16, 2018

Thanks everyone!

@cdeil
Copy link
Member

cdeil commented Apr 17, 2018

This PR broke some tests we have in Gammapy that call assert_quantity_allclose, they now fail with a non-informative AssertionError not saying if the value or unit is wrong:
https://travis-ci.org/gammapy/gammapy/jobs/367497442#L3786

Example:

>>> from astropy.tests.helper import assert_quantity_allclose
>>> import astropy.units as u
>>> assert_quantity_allclose(42 * u.m, 44 * u.m)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/Library/Python/3.6/lib/python/site-packages/astropy/tests/helper.py", line 466, in assert_quantity_allclose
    assert _quantity_allclose(actual, desired, rtol=rtol, atol=atol, **kwargs)
AssertionError

I will just remove all use of Quantity assert helper function calls to Astropy now from Gammapy.
Personally I prefer to have a separate assert on the unit anyways because the unit shouldn't just change unless we change something in our code.

But maybe it's possible to improve this to avoid this breaking change that might affect others?

@@ -460,53 +459,10 @@ def assert_quantity_allclose(actual, desired, rtol=1.e-7, atol=None,
This is a :class:`~astropy.units.Quantity`-aware version of
:func:`numpy.testing.assert_allclose`.
"""
import numpy as np
np.testing.assert_allclose(*_unquantify_allclose_arguments(actual, desired,
Copy link
Member

Choose a reason for hiding this comment

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

It seems this line, which was reinstated because of a comment by @mhvk got deleted again in the sphinx fixes. @pllim can we put this back in your second PR, to fix @cdeil's problem?

Copy link
Member

Choose a reason for hiding this comment

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

Ops. I only saw Tom's comment. Sorry. I'll fix this shortly.

@bsipocz
Copy link
Member

bsipocz commented Apr 17, 2018

Thanks @cdeil for catching this, I wish more packages were testing against the dev version to pick up issues like this so early.

@pllim
Copy link
Member

pllim commented Apr 17, 2018

Wait a minute... My changes here didn't trip any existing tests. And now that I look at the tests, one of them is:

    with pytest.raises(AssertionError):
        assert_quantity_allclose([1, 2] * u.m, [90, 200] * u.cm)

I am confused now, as AssertionError seems to be allowed in some cases. @mhvk ?

@pllim
Copy link
Member

pllim commented Apr 17, 2018

Ah nevermind, it is the extra information... Ignore me. (Need more coffee)

@cdeil
Copy link
Member

cdeil commented Apr 17, 2018

@pllim - ☕️ ☕️ ☕️ ☕️ ☕️ ☕️ ☕️ ☕️

bsipocz pushed a commit that referenced this pull request Apr 19, 2018
bsipocz pushed a commit that referenced this pull request Apr 23, 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.

quantity_allclose can not be imported without pytest installed
8 participants