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

Change return type of `WcsGeom.get_coord()` to quantities #2344

Merged
merged 8 commits into from Sep 6, 2019

Conversation

@adonath
Copy link
Member

commented Sep 6, 2019

This PR addresses #1792. It includes the following changes:

  • Change MapAxis.pix_to_coord() to return a Quantity
  • Change WcsGeom.pix_to_coord() to return Quantities
  • Allow lon and lat to be a Quantityin MapCoord.__init__
  • Adapt tests and notebooks

Shall we declare lon and lat as Longitude and Latitude objects instead of Quantity?

Currently the adaptation of HpxGeom.coord_to_pix() is missing, but if I remember correctly there were problems with the adaptation, when I already tried it ~2 years ago, so I'd prefer to do this in a second PR.

@cdeil cdeil added the cleanup label Sep 6, 2019
@cdeil cdeil added this to the 0.14 milestone Sep 6, 2019
@cdeil cdeil added this to To do in Map analysis via automation Sep 6, 2019
]
},
{
"cell_type": "code",

This comment has been minimized.

Copy link
@cdeil

cdeil Sep 6, 2019

Member

Try make clean-nb?

@cdeil

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

This is great!

Maybe spend an hour or two trying to find code in Gammapy where MapCoords are used and see if they can be simplified now?

At https://nbviewer.jupyter.org/github/gammapy/gammapy/blob/46cea1765477f22fd827565e17124b685af9256a/tutorials/fermi_lat.ipynb#Counts I see a comment

  # The coord-based interface doesn't use Quantity,
   # so we need to pass energy in the same unit as
   # used for the map axis

Can this be deleted or does it still apply?

Not sure how to find those points in the code, probably grepping for "deg" yields too much to review.

Long-term, I think that we should consider changing to rad instead of deg for sky coords.
The WCS interface is deg based, so that's why we have deg at the moment, and that's what's efficient there.
But WCS calls are only made in the setup phase, and then in the model and likelihood loop it's mainly np.sin and np.cos calls, and I think (but didn't benchmark) that those are a bit faster for "rad", because that's what they use internally, i.e. if we use "deg" they convert to "rad" on every call (which is surprisingly cheap, but still, I expect it's an overhead).

@cdeil

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

You could also try changing coords.lon and coords.lat from "deg" to "rad", and see if tests / notebooks fail. Those would be places to fix up.

@adonath

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Thanks @cdeil! I'll merge the PR now (because it's green) and address you comments in a follow-up PR. This way we avoid merge conflicts in gammapy/image/models/tests.

@adonath adonath merged commit 464ce46 into gammapy:master Sep 6, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 5 new issues, 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190906.1 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Map analysis automation moved this from To do to Done Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Map analysis
  
Done
2 participants
You can’t perform that action at this time.