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

Add arithmetic operators for maps #1891

Merged
merged 7 commits into from Nov 2, 2018

Conversation

2 participants
@registerrier
Contributor

registerrier commented Oct 26, 2018

This is a first commit for arithmetics of maps. As suggested by @cdeil we use a general Map._arithmetics function to limit code duplication in the various operators.

Tests check that quantities are correctly handled for various geometries and check that a UnitConversionError is raised if an incorrect operation is attempted.

A proper compatibility check or map geometries will be added later in a subsequent PR.

@registerrier registerrier added this to the 0.9 milestone Oct 26, 2018

@registerrier registerrier self-assigned this Oct 26, 2018

@registerrier registerrier added this to To do in Map analysis via automation Oct 26, 2018

@cdeil

@registerrier - thanks!

See inline comments.

Just out of curiosity, not really needed:
Does this work also if the map is on the right hand side and the quantity is on the left?
If it does, you could add one test showing that it does (e.g. for +).

What about + 4 with a number? Does that work for a unit-less map? Again, if yes, you could add one check that it does.

What about + "spam" with a string? Does this give a TypeError?

@@ -7,7 +7,7 @@
from astropy.coordinates import SkyCoord
import astropy.units as u
from astropy.units import Unit, Quantity
from ...utils.testing import requires_dependency
from ...utils.testing import requires_dependency, assert_quantity_allclose

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

Suggest to use explicit assert on units and values of the results:
https://docs.gammapy.org/0.8/development/howto.html#assert-convention

This comment has been minimized.

@registerrier

This comment has been minimized.

@registerrier

registerrier Oct 26, 2018

Contributor

I have kept some assert_quantity_allclose because for multiplication and division the units can become a bit complex.

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

But if you choose "m" and "s" for units, or make one dimensionless "", it doesn't get very complicated. Note that Unit objects compare with strings:

>>> u.Unit('m s') == 'm s'
True

so in tests to assert units you can just write:

assert m.unit == 'm s'

for the product of "m" and "s".

This comment has been minimized.

@registerrier

registerrier Oct 31, 2018

Contributor

Done

@@ -316,3 +316,49 @@ def test_map_reproject_hpx_to_wcs():
m_r = m.reproject(geom_wcs)
actual = m_r.get_by_coord({"lon": 0, "lat": 0, "energy": [1.0, 3.16227766, 10.0]})
assert_allclose(actual, [287.5, 1055.5, 1823.5], rtol=1e-3)
map_arithmetics_args = [

This comment has been minimized.

@cdeil

cdeil Oct 26, 2018

Member

Arithmetic doesn't do anytihng with binsz, width, center and axes.
Suggest to just pick one map, or if you like one WCS and one HPX.
But even there: what's the point in having both?

This comment has been minimized.

@registerrier

registerrier Oct 26, 2018

Contributor

Well I took them for future geometry equality testing. Probably not needed since this tested already.

Map analysis automation moved this from To do to In progress Oct 26, 2018

@registerrier

This comment has been minimized.

Contributor

registerrier commented Oct 26, 2018

OK.

  • some_map += 4 works well if some_map is dimensionless.
  • 4 + some_map does not work. It results in:
    TypeError: unsupported operand type(s) for +: 'int' and 'WcsNDMap'
  • some_map += 'spam' fails with:
    TypeError: Cannot parse "spam" as a Quantity. It does not start with a number.
    Will do the suggested modifs.

registerrier added some commits Oct 26, 2018

@registerrier

This comment has been minimized.

Contributor

registerrier commented Oct 31, 2018

I have simplified tests.
The __radd__ , __rmul__, __rsub__ and __rtruediv__ will be implemented in a future PR. This requires a bit more work.

@cdeil Merge?

@registerrier

This comment has been minimized.

Contributor

registerrier commented Nov 2, 2018

Travis CI failure unrelated merging now.

@registerrier registerrier merged commit c8638e8 into gammapy:master Nov 2, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 270 new issues, 10 updated code elements – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Map analysis automation moved this from In progress to Done Nov 2, 2018

@cdeil cdeil changed the title from Arithmetics of maps to Add arithmetic operators for maps Nov 23, 2018

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