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

Refactor gammapy.maps methods for calculating index and coordinate arrays #1190

Merged
merged 1 commit into from Nov 5, 2017

Conversation

Projects
None yet
2 participants
@woodmd
Copy link
Member

woodmd commented Nov 3, 2017

This PR is a rewrite of the get_coords() and get_idx() methods of the HPX and WCS geometry classes that changes the return value to a multi-dimensional array. Previously these methods returned flattened arrays with pixels outside the geometry masked. The disadvantage of this design is that it made it difficult to easily retrieve coordinates or indices by slice (e.g. getting coordinates for an image plane or along the non-spatial dimensions at a given image pixel).

The new implementation returns a multi-dimensional array for each coordinate with the same shape as the data member. Pixels outside the geometry are set to NaN (for coords) or -1 (for indices). I believe this implementation will be more intuitive for users as it means there will be a one-to-one mapping of the data array to the arrays returned by these methods. This will also facilitate implementing slice-based operations on the data. Both methods can still return flattened/masked arrays by calling with flat=True.

Note that I plan to make a follow-up PR that updates all set/get methods to gracefully handle NaN inputs.

@cdeil cdeil added the feature label Nov 3, 2017

@cdeil cdeil added this to the 0.7 milestone Nov 3, 2017

@cdeil

cdeil approved these changes Nov 3, 2017

Copy link
Member

cdeil left a comment

@woodmd - Thanks!

Is this ready to be merged from your side?
Note that today in the Gammapy call we discussed that I would release Gammapy v0.7 this Sunday. I don't think it affects your gammapy.maps developments much, but if you want it in for some reason, please merge before sunday.

I didn't review the implementation here, but a quick suggestion: introduce a constant (e.g. module level? or attached to base class?) for the magic value of -1 and make it appear in the docs somehow? (module-level constants that are listed in __all__ should appear).

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Nov 5, 2017

@woodmd - I'm merging this in now.

Just one thought for the future: in xarray and partly also gammapy.cube.SkyCube we use coordinate arrays that don't have the shape of the cube, but just the dimensions they span. I.e. for the sky coordinates 2-dim and e.g. for the energy axis 1-dim. Then in computations those coordinate grids are used so that they broadcast appropriately. This is more memory and probably also CPU-efficient (because for simple computations often the memory to CPU bandwidth is the bottleneck).

So in the future, implementing that for gammapy.maps could be considered, ideally with axis name-based broadcasting. I don't know how much work it is to make that work, so I'm not suggesting to do it here and to hold up this PR for a long time. Merging now.

@cdeil cdeil merged commit 7514318 into gammapy:master Nov 5, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment