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

Development in gammapy.maps #1076

Merged
merged 46 commits into from Jul 11, 2017

Conversation

Projects
None yet
2 participants
@woodmd
Member

woodmd commented Jun 28, 2017

This PR adds new functionality and tests in gammapy.maps. A short summary of some of the changes:

  • Implements interpolation for HpxMapND. Interpolation can be performed by setting interp argument to get_by_coord (analogous to SkyImage.lookup/SkyCube.lookup).
  • Creates pages for documenation in the docs hierarchy. Right now these are mostly empty but I plan to expand them in future PRs.
  • Adds first prototype of sparse Hpx class (HpxMapSparse). Currently it uses classes from scipy.sparse for the internal data representation. More experimentation is probably needed to figure out how to implement get/set/fill methods efficiently.
  • Major refactoring of MapAxis. The implementation is now more in line with LogEnergyAxis -- i.e. a node array is interpolated to perform transformations between pixel and axis coordinates. In principle there is nothing map-specific about this class so this might be a candidate for moving to another submodule.
  • Adds more methods to the abstract interface of MapGeom and MapBase.
@woodmd

This comment has been minimized.

Show comment
Hide comment
@woodmd

woodmd Jun 29, 2017

Member

It appears to be failing on a bug in healpy that is triggered by the combination of the most recent releases of healpy and numpy. It looks like there is already a fix in healpy master but I don't know what the timeline is for a new release. How do you generally handle failures like these? Should it be marked as a known failure?

Member

woodmd commented Jun 29, 2017

It appears to be failing on a bug in healpy that is triggered by the combination of the most recent releases of healpy and numpy. It looks like there is already a fix in healpy master but I don't know what the timeline is for a new release. How do you generally handle failures like these? Should it be marked as a known failure?

@cdeil cdeil self-assigned this Jun 30, 2017

@cdeil cdeil added the feature label Jun 30, 2017

@cdeil cdeil added this to the 0.7 milestone Jun 30, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 30, 2017

Member

It appears to be failing on a bug in healpy that is triggered by the combination of the most recent releases of healpy and numpy. It looks like there is already a fix in healpy master but I don't know what the timeline is for a new release. How do you generally handle failures like these? Should it be marked as a known failure?

I would say yes: mark it as an xfail, and leave a comment with info when / how to remove the xfail in the future (e.g. "this can be removed when we drop support for healpy version XYZ").
In principle you can use either xfail or skipif, and both support only xfailing or skipping under certain conditions, e.g. healpy versions:
https://docs.pytest.org/en/latest/skipping.html

Member

cdeil commented Jun 30, 2017

It appears to be failing on a bug in healpy that is triggered by the combination of the most recent releases of healpy and numpy. It looks like there is already a fix in healpy master but I don't know what the timeline is for a new release. How do you generally handle failures like these? Should it be marked as a known failure?

I would say yes: mark it as an xfail, and leave a comment with info when / how to remove the xfail in the future (e.g. "this can be removed when we drop support for healpy version XYZ").
In principle you can use either xfail or skipif, and both support only xfailing or skipping under certain conditions, e.g. healpy versions:
https://docs.pytest.org/en/latest/skipping.html

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 30, 2017

Member

@woodmd - I had a quick look. The code and diff is very hard to review (not because it's bad, just because it's so much code and nontrivial code). So maybe just you continue here and merge without review, and then once it's in master we start using and reviewing it?
Or if you want me to go through, I can early next week, let me know.

Just very briefly from looking at the docs: the example doesn't work (see https://gist.github.com/cdeil/99b672203c9aa82bd7d9e30167be8842) and WCSMap and WCSMapND are missing a docstring.

Member

cdeil commented Jun 30, 2017

@woodmd - I had a quick look. The code and diff is very hard to review (not because it's bad, just because it's so much code and nontrivial code). So maybe just you continue here and merge without review, and then once it's in master we start using and reviewing it?
Or if you want me to go through, I can early next week, let me know.

Just very briefly from looking at the docs: the example doesn't work (see https://gist.github.com/cdeil/99b672203c9aa82bd7d9e30167be8842) and WCSMap and WCSMapND are missing a docstring.

@woodmd

This comment has been minimized.

Show comment
Hide comment
@woodmd

woodmd Jul 11, 2017

Member

I agree that for now it's probably easiest if I continue pushing changes and we plan for a more thorough review/discussion at a future point. We should also start thinking about how we can integrate this with the SkyImage and SkyCube classes.

With regard to the PR I fixed the outstanding test failures and also fixed the examples in the docs that you noted. For the next PR my plan is to focus on I/O methods.

Member

woodmd commented Jul 11, 2017

I agree that for now it's probably easiest if I continue pushing changes and we plan for a more thorough review/discussion at a future point. We should also start thinking about how we can integrate this with the SkyImage and SkyCube classes.

With regard to the PR I fixed the outstanding test failures and also fixed the examples in the docs that you noted. For the next PR my plan is to focus on I/O methods.

@woodmd woodmd merged commit 09f1bd4 into gammapy:master Jul 11, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 11, 2017

Member

@woodmd - Thanks!

Note that the Gammapy docs build on RTD is broken at the moment, so docs for this won't show up online yet. See rtfd/readthedocs.org#2827 (I'll try to play around with that further to resolve the issue soon when I get a chance.)

Member

cdeil commented Jul 11, 2017

@woodmd - Thanks!

Note that the Gammapy docs build on RTD is broken at the moment, so docs for this won't show up online yet. See rtfd/readthedocs.org#2827 (I'll try to play around with that further to resolve the issue soon when I get a chance.)

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 11, 2017

Member

Some whitespace cleanup in 4cb9e10 in master.

Member

cdeil commented Jul 11, 2017

Some whitespace cleanup in 4cb9e10 in master.

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