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

Fix Gauss PSF energy bin issue #778

Merged
merged 5 commits into from Nov 17, 2016

Conversation

Projects
None yet
4 participants
@JouvinLea
Contributor

JouvinLea commented Nov 16, 2016

This PR change the way you find the sigmas and norm of the psf for a given energy. Before it was on the self.energy_high and it was giving some mistakes... This is why the full corrected was not working since it didn't take the parameters of the good corresponding energy bin.

Lea Jouvin added some commits Nov 16, 2016

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Nov 16, 2016

Contributor

@cdeil
In order for the test to pass, I made a PR in gammapy_extra for the psf reference file for HAP

Contributor

JouvinLea commented Nov 16, 2016

@cdeil
In order for the test to pass, I made a PR in gammapy_extra for the psf reference file for HAP

Lea Jouvin
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi
Contributor

joleroi commented Nov 17, 2016

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Nov 17, 2016

Contributor

also related #570

Contributor

joleroi commented Nov 17, 2016

also related #570

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 17, 2016

Member

@JouvinLea The fix looks correct to me.

@cdeil Is there a reason, why ENERG_LO and ENERG_HI are stored in the triple Gauss PSF format, when in fact the values of the PSF are given at the bin center? This causes confusion, that is reflected in the EnergyDependentMultiGaussPSF energy_hi and energy_lo attributes, which should probably removed..

Member

adonath commented Nov 17, 2016

@JouvinLea The fix looks correct to me.

@cdeil Is there a reason, why ENERG_LO and ENERG_HI are stored in the triple Gauss PSF format, when in fact the values of the PSF are given at the bin center? This causes confusion, that is reflected in the EnergyDependentMultiGaussPSF energy_hi and energy_lo attributes, which should probably removed..

@cdeil cdeil added the cleanup label Nov 17, 2016

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

@cdeil cdeil self-assigned this Nov 17, 2016

@cdeil cdeil changed the title from Tripple Gauss PSF issue to Fix Gauss PSF energy bin issue Nov 17, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 17, 2016

Member

@JouvinLea - Thanks!

+1 to merge this once travis-ci passes, I've restarted the build just now after merging gammapy/gammapy-extra#43 .

@adonath - For now, we have this:
http://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/psf/psf_3gauss/index.html
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDependentMultiGaussPSF.html

For the file format, this has been discussed, and the conclusion was that if someone wants to specify the exact node at which the PSF is evaluated, they should fill ENERG_LO = ENERG_HI.
@JouvinLea - That would be my recommendation that you do that in the HAP-FR exporters.

We can also discuss whether it makes sense to expose the energy lo / hi fields or make them private. I'm not sure dropping the info completely and only storing the center on read is a good solution. @joleroi might also have an opinion on this?

IMO it's not so bad as-is. Maybe a generic extension on a data member or attribute for IRF axes on the node point would be good, and it's set to log center for energy axes by default?

Member

cdeil commented Nov 17, 2016

@JouvinLea - Thanks!

+1 to merge this once travis-ci passes, I've restarted the build just now after merging gammapy/gammapy-extra#43 .

@adonath - For now, we have this:
http://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/psf/psf_3gauss/index.html
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDependentMultiGaussPSF.html

For the file format, this has been discussed, and the conclusion was that if someone wants to specify the exact node at which the PSF is evaluated, they should fill ENERG_LO = ENERG_HI.
@JouvinLea - That would be my recommendation that you do that in the HAP-FR exporters.

We can also discuss whether it makes sense to expose the energy lo / hi fields or make them private. I'm not sure dropping the info completely and only storing the center on read is a good solution. @joleroi might also have an opinion on this?

IMO it's not so bad as-is. Maybe a generic extension on a data member or attribute for IRF axes on the node point would be good, and it's set to log center for energy axes by default?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 17, 2016

Member

@cdeil Thanks for the explanation!

For SkyCube, I've removed all hard coded .energy attributes and used the LogEnergyAxis object instead. Energies can than be computed on the fly by calling the method SkyCube.energies(mode=['center', 'edges']), which returns the corresponding values. This approach is equivalent to SkyImage.coordinates(mode=['center', 'edges']), which uses WCS instead of LogEnergyAxis. To me this seemed to be the most intuitive (and WCS compatible...) API, instead of having additional attributes for e.g. SkyImage.coordinates_x_hi or SkyImage.coordinates_x_lo. I'd prefer a similar solution for IRF energy axes. I.e. don't have hard coded energy bins specification attached to the class, but only have a transformation object, the knows how to correctly convert between pixel and world coordinates. This object is needed anyway, when interpolation is used.

Member

adonath commented Nov 17, 2016

@cdeil Thanks for the explanation!

For SkyCube, I've removed all hard coded .energy attributes and used the LogEnergyAxis object instead. Energies can than be computed on the fly by calling the method SkyCube.energies(mode=['center', 'edges']), which returns the corresponding values. This approach is equivalent to SkyImage.coordinates(mode=['center', 'edges']), which uses WCS instead of LogEnergyAxis. To me this seemed to be the most intuitive (and WCS compatible...) API, instead of having additional attributes for e.g. SkyImage.coordinates_x_hi or SkyImage.coordinates_x_lo. I'd prefer a similar solution for IRF energy axes. I.e. don't have hard coded energy bins specification attached to the class, but only have a transformation object, the knows how to correctly convert between pixel and world coordinates. This object is needed anyway, when interpolation is used.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 17, 2016

Member

I'm merging this PR now.

I'd prefer a similar solution for IRF energy axes. I.e. don't have hard coded energy bins specification attached to the class, but only have a transformation object, the knows how to correctly convert between pixel and world coordinates. This object is needed anyway, when interpolation is used.

@adonath and @joleroi - let's continue the discussion in #776 or a separate issue.

Just a note: the second analytical IRF we have is the King PSF class, which currently does this:
http://docs.gammapy.org/en/latest/api/gammapy.irf.PSFKing.html#gammapy.irf.PSFKing.evaluate
whereas for the histogram PSF we do this:
http://docs.gammapy.org/en/latest/_modules/gammapy/irf/psf_3d.html#PSF3D.evaluate

Probably it's best to always start by transforming analytical IRFs to histogram IRFs and then do all interpolation or other computations on those, using the generic IRF scheme @joleroi has started.

Member

cdeil commented Nov 17, 2016

I'm merging this PR now.

I'd prefer a similar solution for IRF energy axes. I.e. don't have hard coded energy bins specification attached to the class, but only have a transformation object, the knows how to correctly convert between pixel and world coordinates. This object is needed anyway, when interpolation is used.

@adonath and @joleroi - let's continue the discussion in #776 or a separate issue.

Just a note: the second analytical IRF we have is the King PSF class, which currently does this:
http://docs.gammapy.org/en/latest/api/gammapy.irf.PSFKing.html#gammapy.irf.PSFKing.evaluate
whereas for the histogram PSF we do this:
http://docs.gammapy.org/en/latest/_modules/gammapy/irf/psf_3d.html#PSF3D.evaluate

Probably it's best to always start by transforming analytical IRFs to histogram IRFs and then do all interpolation or other computations on those, using the generic IRF scheme @joleroi has started.

@cdeil cdeil merged commit ec9fb87 into gammapy:master Nov 17, 2016

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Nov 17, 2016

Contributor

Probably it's best to always start by transforming analytical IRFs to histogram IRFs and then do all interpolation or other computations on those,

Yes, as far as I remember we already agreed on this in the past.

Contributor

joleroi commented Nov 17, 2016

Probably it's best to always start by transforming analytical IRFs to histogram IRFs and then do all interpolation or other computations on those,

Yes, as far as I remember we already agreed on this in the past.

@joleroi joleroi referenced this pull request Nov 21, 2016

Merged

Update test HESS chains #768

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