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 misc issues with IRF classes #395

Merged
merged 6 commits into from Dec 3, 2015

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Dec 2, 2015

This fixes an issue raised by @cdeil and @JouvinLea
see gammasky/hess-host-analyses#23

@joleroi joleroi added the bug label Dec 2, 2015

@joleroi joleroi self-assigned this Dec 2, 2015

# Check normalization
actual = np.sum(response)
desired = 1
assert_allclose(actual, desired, rtol=1e-1)

This comment has been minimized.

@joleroi

joleroi Dec 2, 2015

Contributor

@cdeil : Do we need a better agreement with 1 here?
I can't judge if this is a binning effect or something else. I could force this to be 1 by renormalizing of course.

@joleroi

joleroi Dec 2, 2015

Contributor

@cdeil : Do we need a better agreement with 1 here?
I can't judge if this is a binning effect or something else. I could force this to be 1 by renormalizing of course.

This comment has been minimized.

@cdeil

cdeil Dec 2, 2015

Member

Short answer: I don't know. We'll have to discuss / investigate where edisp PDF normalisation is important.

I think at the moment Sherpa is applying the energy dispersion, and it's a black box / not clear if the inputs need to be normalised and with what precision, no?
Maybe the RMF format has something to say about normalisation?

In the end normalisation at the ~ 1% level is important, to get the correct number of predicted counts per reco bin for a given source spectrum, i.e. a flux amplitude that is correct at the ~ 1% level.
What's not so important is the exact shape of the dispersion, i.e. pixel binning / interpolation effects.
So my understanding is that it's OK to have low-precision edisp stored and interpolated onto the RMF matrix, and only then than that RMF matrix has to be normalised (unless Sherpa is doing that, then we have to do nothing).

This is something we can discuss with Regis on Friday.

It would be good if you can find out what Sherpa does / the RMF format says, or write some test scripts, e.g. for a simulated power-law spectrum or spectral line (e.g. modeled as a Gaussian in log(Energy) or Energy) ... can you get correct npred and flux for those if some energy resolution is involved?

@cdeil

cdeil Dec 2, 2015

Member

Short answer: I don't know. We'll have to discuss / investigate where edisp PDF normalisation is important.

I think at the moment Sherpa is applying the energy dispersion, and it's a black box / not clear if the inputs need to be normalised and with what precision, no?
Maybe the RMF format has something to say about normalisation?

In the end normalisation at the ~ 1% level is important, to get the correct number of predicted counts per reco bin for a given source spectrum, i.e. a flux amplitude that is correct at the ~ 1% level.
What's not so important is the exact shape of the dispersion, i.e. pixel binning / interpolation effects.
So my understanding is that it's OK to have low-precision edisp stored and interpolated onto the RMF matrix, and only then than that RMF matrix has to be normalised (unless Sherpa is doing that, then we have to do nothing).

This is something we can discuss with Regis on Friday.

It would be good if you can find out what Sherpa does / the RMF format says, or write some test scripts, e.g. for a simulated power-law spectrum or spectral line (e.g. modeled as a Gaussian in log(Energy) or Energy) ... can you get correct npred and flux for those if some energy resolution is involved?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 2, 2015

Contributor

@cdeil

There are many interpolation functions in the EnergyDispersion class that are not implemented. I don't think we ever need them, so I will remove them if you don't mind

Contributor

joleroi commented Dec 2, 2015

@cdeil

There are many interpolation functions in the EnergyDispersion class that are not implemented. I don't think we ever need them, so I will remove them if you don't mind

@joleroi joleroi changed the title from Fix EnergyDispersion2D.to_energy_dispersion to Fix Issues in IRF classes Dec 2, 2015

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 3, 2015

Member

@joleroi - Could you please have a look at this?

  • Could you please add an info() method to EffectiveAreaTable2D?
  • Do you also see the error from aeff.peek() in cell 7? Is that something in Gammapy that should be fixed or a problem with the FITS file or am I running some old version of Gammapy?

If there's something to fix / change in Gammapy, maybe you could do it here and merge this PR soon?

Member

cdeil commented Dec 3, 2015

@joleroi - Could you please have a look at this?

  • Could you please add an info() method to EffectiveAreaTable2D?
  • Do you also see the error from aeff.peek() in cell 7? Is that something in Gammapy that should be fixed or a problem with the FITS file or am I running some old version of Gammapy?

If there's something to fix / change in Gammapy, maybe you could do it here and merge this PR soon?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 3, 2015

Contributor

It was a bug in gammapy (that I certainly did not introduce 😃 )

Contributor

joleroi commented Dec 3, 2015

It was a bug in gammapy (that I certainly did not introduce 😃 )

joleroi added a commit that referenced this pull request Dec 3, 2015

@joleroi joleroi merged commit f8aa7ea into gammapy:master Dec 3, 2015

1 check passed

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

@joleroi joleroi deleted the joleroi:edisp_fix branch Dec 3, 2015

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2015

Member

@joleroi - Thanks for fixing my bugs! :-)

Everything seems to work / looks OK to me now: see updated hap-fr-first-look.ipynb

Member

cdeil commented Dec 4, 2015

@joleroi - Thanks for fixing my bugs! :-)

Everything seems to work / looks OK to me now: see updated hap-fr-first-look.ipynb

@cdeil cdeil changed the title from Fix Issues in IRF classes to Fix misc issues with IRF classes Dec 12, 2015

@cdeil cdeil added the cleanup label Feb 20, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 20, 2016

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