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

Use assert_quantity_allclose from Astropy #306

Merged
merged 3 commits into from Jul 21, 2015

Conversation

Projects
None yet
2 participants
@mapazarr
Member

mapazarr commented Jul 21, 2015

This function: gammapy.utils.testing.assert_quantity seems to be a duplication of astropy.tests.helper.assert_quantity_allclose. Maybe we don't need it?

If changed/removed, the docs here should be updated too: http://gammapy.readthedocs.org/en/latest/development/index.html
(the entry for the assert section (at the end of the mentioned page) is as of now missing, because it is being added in PR #299, still not merged).

Triggered by this astropy issue: astropy/astropy#3978

@mapazarr mapazarr referenced this pull request Jul 20, 2015

Merged

Add cube background model class #299

8 of 8 tasks complete

@cdeil cdeil added the question label Jul 20, 2015

@cdeil cdeil added this to the 0.3 milestone Jul 20, 2015

@cdeil cdeil self-assigned this Jul 20, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 20, 2015

@mapazarr – Yes. Please remove the version from Gammapy and replace all callers (if any) with the version from Astropy.
There's a ton of stuff in Gammapy that has become available in Astropy or affiliated packages like reproject, photutils, ... in the past two years and should be cleaned up. It's great if you help with this.

Some suggestions for the future to make issues / PRs quicker to review for maintainers in the future:

  • Link in the issue / PR description to the relevant docs or code. In this case the astropy.tests.helper.assert_quantity_allclose and gammapy.utils.testing.assert_quantity functions.
  • There's a command line tool called hub that lets you turn a Github issue into a pull request. Make a branch, push it to your fork and then run the hub pull-request -i XXX command. I often use this to attach a solution to an issue, instead of opening a separate pull request and spreading the discussion across two places. Maybe give it a try with this issue?
@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 20, 2015

Ok, assign it to me please.

@cdeil cdeil assigned mapazarr and unassigned cdeil Jul 20, 2015

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 21, 2015

@cdeil I used the hub command as suggested, and I get:
Warning: Issue to pull request conversion is deprecated and might not work in the future.
So maybe another solution should be found in the future for converting issues into PRs.

assert actual.unit == desired.unit
assert_allclose(actual, desired, *args, **kwargs)
__all__ = []

This comment has been minimized.

@mapazarr

mapazarr Jul 21, 2015

Member

This file is now basically empty: no methods are defined. Should we:

  • keep it in case we need to implement custom tests in the future?
  • or should we remove it?

This comment has been minimized.

@cdeil

cdeil Jul 21, 2015

Member

Just remove it for now ... we can always add it back later if we need it.
There might also be a Sphinx automodapi directive you have to remove in the docs.
And you can use the Github full-text search to make sure you've replaced all the callers (test coverage in Gammapy is so bad that you can't rely on tests passing)

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 21, 2015

I've been getting that hub warning for years now.
I don't know why Github has deprecated that procedure of turning issues into PRs, I think it's very nice to have issue report, discussion and solution in one ticket instead of two.

`astropy.tests.helper.assert_quantity_allclose`.
In this case, use
``from astropy.tests.helper import assert_quantity_allclose``

This comment has been minimized.

@cdeil

cdeil Jul 21, 2015

Member

I think this will render more nicely if you do this, no?

.. code-block:: python
    from astropy.tests.helper import assert_quantity_allclose

This comment has been minimized.

@mapazarr
@@ -162,7 +161,7 @@ def test_integral_flux_image(self):
def test_solid_angle_image(self):
actual = self.spectral_cube.solid_angle_image[10][30]
expected = Quantity(self.spectral_cube.wcs.wcs.cdelt[:-1].prod(), 'deg2')
assert_quantity(actual, expected.to('sr'), rtol=1e-4)
assert_quantity_allclose(actual, expected.to('sr'), rtol=1e-4)

This comment has been minimized.

@cdeil

cdeil Jul 21, 2015

Member

The .to('sr') part is superfluous and can be removed, no?
I.e. the same test with the same precision is performed if it's there or not?

This comment has been minimized.

@mapazarr

mapazarr Jul 21, 2015

Member

Removed.

@@ -33,7 +32,7 @@ def test_psf(self):
energy = Quantity(100, 'GeV')
fraction = 0.68
angle = psf.containment_radius(energy, fraction)
assert_quantity(angle, Angle('0.1927459865412511 deg'))
assert_quantity_allclose(angle, Angle('0.1927459865412511 deg'))

This comment has been minimized.

@cdeil

cdeil Jul 21, 2015

Member

Please change to this format:

Angle(0.1927459865412511, 'deg')

It's the same, but let's try to be consistent in our use of Angle, Quantity, ... if you come across other similar cases, feel free to change those to this format as well.

This comment has been minimized.

@mapazarr

mapazarr Jul 21, 2015

Member

Fixed.

@@ -71,7 +70,7 @@ def test_psf(self):
energy = Quantity(100, 'GeV')
fraction = 0.68
angle = psf.containment_radius(energy, fraction)
assert_quantity(angle, Angle('0.13185321269896136 deg'))
assert_quantity_allclose(angle, Angle('0.13185321269896136 deg'))

This comment has been minimized.

@cdeil

cdeil Jul 21, 2015

Member

Same here ... let's use Angle(0.13185321269896136, 'deg')

This comment has been minimized.

@mapazarr

mapazarr Jul 21, 2015

Member

Done.
Using actually 'degree', I thought you preferred that way.

@cdeil cdeil changed the title from assert_quantity might be a duplication of assert_quantity_allclose to Use assert_quantity_allclose from Astropy Jul 21, 2015

@cdeil cdeil added cleanup tests and removed question labels Jul 21, 2015

@cdeil cdeil assigned cdeil and unassigned mapazarr Jul 21, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 21, 2015

I've left some minor comments ... once these are addressed this is OK.

I've also changed the title of the PR and the labels.
As assignee I've put myself following the Astropy workflow of putting the person that does the main review and eventually will hit the merge button.
For issues it makes sense to put the implementer and I'm sometimes doing this, though.
Not 100% sure what the best workflow is here, but Gammapy is still a pretty small project and it doesn't matter much.

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 21, 2015

Isn't it possible to add 2 assignee? The one implementing the changes and the one reviewing them?

Anyway. I implemented the changes.

mapazarr added some commits Jul 21, 2015

cdeil added a commit that referenced this pull request Jul 21, 2015

Merge pull request #306 from mapazarr/assert_quantity_cleanup
Use assert_quantity_allclose from Astropy

@cdeil cdeil merged commit 7f94830 into gammapy:master Jul 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Member

cdeil commented Jul 21, 2015

There's only 1 assignee possible.
The Github issue / PR system is intentionally kept super-simple (compared to other issue trackers like e.g. JIRA that we use in HESS).
This simplicity is great 99% of the time, but sometimes more options or customisation would be nice to have...

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 21, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment