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

Improve SkyCube energy axes #759

Merged
merged 8 commits into from Nov 10, 2016

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Nov 9, 2016

This PR is the second part of a major rework of the SkyCube class. These are the mos important changes:

  • Rename SkyCube.integral_flux_image() to SkyCube.sky_image_integral(), and reimplement the method using _trapz_loglog.
  • Add the interpolation options back and modify the interpolation to work in log-log
  • Introduce the format options 'fermi-counts', 'fermi-background' and 'fermi-exposure'in SkyCube.read(), because in the previous implementation units and energy ranges were not read and set correctly.
  • Replace SkyCube.energy by SkyCube.energies() which has the options 'center' and 'edges'. This was done because the former SkyCube.energy was not well defined. In some cases it represented the edges in some cases the centers.
  • Minor: Rename SkyCube.ref_sky_image to SkyCube.sky_image_ref. This has the advantage that all sky image related properties and methods start with sky_image_... and will be listed by tab completion.
  • Add more interpolation and reprojection tests in general

@adonath adonath self-assigned this Nov 9, 2016

adonath added some commits Nov 9, 2016

@cdeil cdeil added this to the 0.5 milestone Nov 9, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 9, 2016

Member

There's one test fail that's probably related to this PR:
https://travis-ci.org/gammapy/gammapy/jobs/174551958#L979

Otherwise: 👍 to merge.
(I didn't review it and don't have time this week.)

Member

cdeil commented Nov 9, 2016

There's one test fail that's probably related to this PR:
https://travis-ci.org/gammapy/gammapy/jobs/174551958#L979

Otherwise: 👍 to merge.
(I didn't review it and don't have time this week.)

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 10, 2016

Member

The remaining test fails will be fixed with gammapy/gammapy-extra#42.

@leajouvin Please note that this PR changes the fit results from the sherpa cube fit notebook, because of a fixed bug in the energy boundary computation, when SkyCube is transformed to the sherpa data object. It might affect your analysis results as well.

Member

adonath commented Nov 10, 2016

The remaining test fails will be fixed with gammapy/gammapy-extra#42.

@leajouvin Please note that this PR changes the fit results from the sherpa cube fit notebook, because of a fixed bug in the energy boundary computation, when SkyCube is transformed to the sherpa data object. It might affect your analysis results as well.

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Nov 10, 2016

Contributor

@adonath
I don't see really the difference between what you implemented and what was before. I mean in my script I am already giving the edges for the energy bin so there was no bug no?

Contributor

JouvinLea commented Nov 10, 2016

@adonath
I don't see really the difference between what you implemented and what was before. I mean in my script I am already giving the edges for the energy bin so there was no bug no?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 10, 2016

Member

@leajouvin Yes, depending on how you gave the energy binning to the cube or read it from file, it might give different results or not. It did for the sherpa cube analysis notebook. I just wanted to make you aware of this change, in case you see any issues with your analysis.

Member

adonath commented Nov 10, 2016

@leajouvin Yes, depending on how you gave the energy binning to the cube or read it from file, it might give different results or not. It did for the sherpa cube analysis notebook. I just wanted to make you aware of this change, in case you see any issues with your analysis.

@adonath adonath merged commit 2c6faf6 into gammapy:master Nov 10, 2016

2 checks passed

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

@adonath adonath deleted the adonath:rework_cube_energy_axes branch Nov 10, 2016

@adonath adonath referenced this pull request Nov 14, 2016

Closed

Finish SkyCube implementation #121

6 of 8 tasks complete

@cdeil cdeil changed the title from Rework cube energy axes to Improve SkyCube energy axes Nov 18, 2016

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