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 Map.get_coord to return a MapCoord object #1395

Merged
merged 5 commits into from May 3, 2018

Conversation

3 participants
@registerrier
Contributor

registerrier commented Apr 24, 2018

At the moment ~gammapy.maps.Geomhas only a get_coord()methods that yields a tuple of arrays.
It is desirable to have a MapCoord containing all coordinates contained in a Geom object.
This PR adds a get_mapcoord() method to do so.

@registerrier registerrier added this to the 0.8 milestone Apr 24, 2018

@registerrier registerrier self-assigned this Apr 24, 2018

@registerrier registerrier added this to To do in Map analysis via automation Apr 24, 2018

@registerrier registerrier requested review from cdeil and woodmd Apr 24, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 24, 2018

Member

@registerrier - Thanks!

Looking around a bit, I found three methods on geom (and none on map) that return coords:

I'm +1 to use MapCoord as much as possible, because there the coords have names and units, and nice convenience methods attached. If a user really needs / wants tuples, then we can add a to_tuple method on MapCoord, no?

The alternative is to have separate get_coord and get_mapcoord and similar methods returning coords forever, which is more confusing than helpful for users, no?
@registerrier @woodmd - Thoughts?

@registerrier - probably you want to get this PR in tomorrow before you go on vacation?
Do you have time to add a simple unit test with an example and some asserts on what is filled in the return value? I think some things like e.g. coords units aren't filled correctly. That's OK, you can leave TODO comment and then we improve in the coming days.

Member

cdeil commented Apr 24, 2018

@registerrier - Thanks!

Looking around a bit, I found three methods on geom (and none on map) that return coords:

I'm +1 to use MapCoord as much as possible, because there the coords have names and units, and nice convenience methods attached. If a user really needs / wants tuples, then we can add a to_tuple method on MapCoord, no?

The alternative is to have separate get_coord and get_mapcoord and similar methods returning coords forever, which is more confusing than helpful for users, no?
@registerrier @woodmd - Thoughts?

@registerrier - probably you want to get this PR in tomorrow before you go on vacation?
Do you have time to add a simple unit test with an example and some asserts on what is filled in the return value? I think some things like e.g. coords units aren't filled correctly. That's OK, you can leave TODO comment and then we improve in the coming days.

@woodmd

This comment has been minimized.

Show comment
Hide comment
@woodmd

woodmd Apr 25, 2018

Member

I'd be in favor of changing the return type of get_coord to MapCoord rather than adding a new method. I considered doing this earlier but held off since it entails a lot of work to update all the examples and unit tests. Because MapCoord objects support tuple-style access I think the changes needed elsewhere in the code may not actually be that extensive.

Member

woodmd commented Apr 25, 2018

I'd be in favor of changing the return type of get_coord to MapCoord rather than adding a new method. I considered doing this earlier but held off since it entails a lot of work to update all the examples and unit tests. Because MapCoord objects support tuple-style access I think the changes needed elsewhere in the code may not actually be that extensive.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 25, 2018

Member

+1 to change get_coord to return MapCoords.
I think that also means that WcsGeom.coord_to_tuple can be deleted, possibly adapting the two callers:

gammapy/maps/geom.py
1214:    def coord_to_tuple(self, coord):

gammapy/maps/hpx.py
764:        c = self.coord_to_tuple(coords)

gammapy/maps/wcs.py
478:        c = self.coord_to_tuple(coords)
Member

cdeil commented Apr 25, 2018

+1 to change get_coord to return MapCoords.
I think that also means that WcsGeom.coord_to_tuple can be deleted, possibly adapting the two callers:

gammapy/maps/geom.py
1214:    def coord_to_tuple(self, coord):

gammapy/maps/hpx.py
764:        c = self.coord_to_tuple(coords)

gammapy/maps/wcs.py
478:        c = self.coord_to_tuple(coords)
@registerrier

This comment has been minimized.

Show comment
Hide comment
@registerrier

registerrier Apr 25, 2018

Contributor

I have changed the WcsGeom.get_coord() and HpxGeom.get_coord() methods to return directly a MapCoord. This seems to work fine since MapCoord supports tuple-like access.

There are a number of remaining failed tests mostly due to WcsNDMap.interp_by_coord() when the latter calls griddata. I'll try to figure out what is going wrong latter.

Contributor

registerrier commented Apr 25, 2018

I have changed the WcsGeom.get_coord() and HpxGeom.get_coord() methods to return directly a MapCoord. This seems to work fine since MapCoord supports tuple-like access.

There are a number of remaining failed tests mostly due to WcsNDMap.interp_by_coord() when the latter calls griddata. I'll try to figure out what is going wrong latter.

Fix coord handling in _interp_by_coord_griddata
Now that the coords are MapCoord objects,
we need to cast to tuple explicitly.
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 2, 2018

Member

I've added 4a7e17f fixing the call to scipy griddata.

Overall I'm not sure yet if we have a sane way to work with coords. This PR introduces several tuple(map_coords) calls the codebase and tests, and overall coords-related code is pretty complex, no? At least to me it's not clear what our strategy is where to use MapCoord or tuple objects in Gammapy, and how to review / evolve the Gammapy codebase to get there.

Is this the goal?

  • The goal is to use MapCoord as much as possible, internally and in return values.
  • We allow tuple input, but always cast to MapCoord on the first line in methods where we allow coord tuple input (this is cheap)
  • We only cast to tuple where needed, usually when calling into numpy / scipy / healpy

Even if this were agreed, I'm still not sure how to get there.
One idea could be to remove MapCoord.__iter__ and to add a method map_coord.to_tuple() which does this, to be more explicit and able to find all lines where we convert MapCoord to tuple.
I guess finding all methods that take coords as input can be done via code reading or search, and we add coord = MapCoord.from_any(coord) lines there at the top of each one (which are pass-through for MapCoord objects, and create MapCoord if tuple or dict are passed).

@woodmd @registerrier - Please review this PR and share your thoughts.

Member

cdeil commented May 2, 2018

I've added 4a7e17f fixing the call to scipy griddata.

Overall I'm not sure yet if we have a sane way to work with coords. This PR introduces several tuple(map_coords) calls the codebase and tests, and overall coords-related code is pretty complex, no? At least to me it's not clear what our strategy is where to use MapCoord or tuple objects in Gammapy, and how to review / evolve the Gammapy codebase to get there.

Is this the goal?

  • The goal is to use MapCoord as much as possible, internally and in return values.
  • We allow tuple input, but always cast to MapCoord on the first line in methods where we allow coord tuple input (this is cheap)
  • We only cast to tuple where needed, usually when calling into numpy / scipy / healpy

Even if this were agreed, I'm still not sure how to get there.
One idea could be to remove MapCoord.__iter__ and to add a method map_coord.to_tuple() which does this, to be more explicit and able to find all lines where we convert MapCoord to tuple.
I guess finding all methods that take coords as input can be done via code reading or search, and we add coord = MapCoord.from_any(coord) lines there at the top of each one (which are pass-through for MapCoord objects, and create MapCoord if tuple or dict are passed).

@woodmd @registerrier - Please review this PR and share your thoughts.

@registerrier

This comment has been minimized.

Show comment
Hide comment
@registerrier

registerrier May 2, 2018

Contributor

Looks fine to me. Thanks for solving the issue.

I tend to agree that we would like to use mostly MapCoords rather than tuple at least externally to interact with maps. I don't know how much work is needed to really make everything work with MapCoord internally. A lot I guess.

Also I am not sure this is something desirable. MapCoord would be a more efficient tool if it where able to deal with ~astropy.units and coordinates in a more general way (i.e. independently of a specific Map and Geom). It could provide a way to interact with Map and Geom in a more unit friendly way.

Contributor

registerrier commented May 2, 2018

Looks fine to me. Thanks for solving the issue.

I tend to agree that we would like to use mostly MapCoords rather than tuple at least externally to interact with maps. I don't know how much work is needed to really make everything work with MapCoord internally. A lot I guess.

Also I am not sure this is something desirable. MapCoord would be a more efficient tool if it where able to deal with ~astropy.units and coordinates in a more general way (i.e. independently of a specific Map and Geom). It could provide a way to interact with Map and Geom in a more unit friendly way.

Show outdated Hide outdated gammapy/maps/hpx.py
Show outdated Hide outdated gammapy/maps/wcs.py
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 3, 2018

Member

MapCoord would be a more efficient tool if it where able to deal with ~astropy.units and coordinates in a more general way (i.e. independently of a specific Map and Geom). It could provide a way to interact with Map and Geom in a more unit friendly way.

@registerrier - I think we'll just have to improve MapCoord step by step and the Gammapy code base as well. If you have any specific suggestions to change or add something, please open an issue any time.

Member

cdeil commented May 3, 2018

MapCoord would be a more efficient tool if it where able to deal with ~astropy.units and coordinates in a more general way (i.e. independently of a specific Map and Geom). It could provide a way to interact with Map and Geom in a more unit friendly way.

@registerrier - I think we'll just have to improve MapCoord step by step and the Gammapy code base as well. If you have any specific suggestions to change or add something, please open an issue any time.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 3, 2018

Member

Fails are unrelated (#1403 (comment)). Merging this now.

@registerrier - Thanks!

Obviously this is just the start, more work is needed on map coordinates.

Member

cdeil commented May 3, 2018

Fails are unrelated (#1403 (comment)). Merging this now.

@registerrier - Thanks!

Obviously this is just the start, more work is needed on map coordinates.

@cdeil

cdeil approved these changes May 3, 2018

@cdeil cdeil merged commit 8eb04cf into gammapy:master May 3, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

Map analysis automation moved this from To do to Done May 3, 2018

@cdeil cdeil changed the title from Add get_mapcoord to gammapy.maps.Geom to Change Map.get_coord to return a MapCoord object Aug 15, 2018

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