cta-observatory / pyirf

Python IRF builder
https://pyirf.readthedocs.io/en/stable/
MIT License
15 stars 25 forks source link

Add function to transform from energy dispersion to energy migration #48

Open maxnoe opened 4 years ago

maxnoe commented 4 years ago

Energy dispersion is a better format for storage, as it is less sparse, but for actual computations, one needs energy migration.

We should have a function that transforms the energy dispersion matrix (Bins true energy, reco_energy / true_energy) to migration matrix (bins true_energy, reco_energy).

This is needed to estimate sensitivity from IRFs

LukasNickel commented 4 years ago

So the difference is just in the value of the migration bins? The documentation uses these two terms interchangeably btw: https://cta-observatory.github.io/pyirf/irf/index.html

maxnoe commented 4 years ago

Yes, we should make a clear distinction.

The difference is the second axis of the IRF, yes. One is the relative change one is absolute reconstructed energy.

HealthyPear commented 4 years ago

perhaps the associated modification to the docs can be done together with the addition of this function

LukasNickel commented 4 years ago

Is there an official document, that we could link for that?

maxnoe commented 4 years ago

@vuillaut I don't think this has anything to do with full enclosure irfs. It's not needed for the computation of the IRFs and should be the same procedure for point-like as for full-enclosure IRFs.

maxnoe commented 4 years ago

Is there an official document, that we could link for that?

I don't think so. But there should be code in gammapy that does this transformation.

Maybe @adonath can give you a pointer

HealthyPear commented 4 years ago

This ambiguity is also found in the OGADF website: energy dispersion is defined as the PDF of energy migration

vuillaut commented 4 years ago

@vuillaut I don't think this has anything to do with full enclosure irfs. It's not needed for the computation of the IRFs and should be the same procedure for point-like as for full-enclosure IRFs.

Don't we want to compute the sensitivity from IRFs at the end? Ok not directly related.

HealthyPear commented 4 years ago

Don't we want to compute the sensitivity from IRFs at the end?

Indeed, but I believe this is only important for cut optimizations which are not best-sensitivity (in that case the sensitivity depends on the specific optimized IRF no?)

Since we are now optimizing for best-point-source sensitivity, the sensitivity from the IRFs should coincide with that form the optimized cuts.

maxnoe commented 4 years ago

Don't we want to compute the sensitivity from IRFs at the end? Ok not directly related.

Yes, it's a TODO for the sensitivity calculation from IRFs

LukasNickel commented 4 years ago

Energy dispersion is a better format for storage, as it is less sparse

This is because the transformation introduces additional bins outside of the "dispersion range" for a given true energy, right? So there are a lot of new bin combinations, that just remain empty.

maxnoe commented 4 years ago

Yes, basically you never expect an event to go from 100 GeV to 100 TeV. But if you have a migration matrix with both ranges from 10 GeV to 100 TeV you will have to store also these bins.

adonath commented 4 years ago

In Gammapy we use the term "energy dispersion" (or EDisp...) as the general term for the energy resolution. The actual implementation is either handled with the migration PDF (e.g. for event simulation), or an integrated matrix for the forward folding (that we either call RMF or EDispKernel matrix). We have a docs page to explain the energy dispersion, but we still have to fill it (https://docs.gammapy.org/0.17/irf/edisp.html)...

@MaxNoe In Gammapy we have code to integrate the energy migration PDF in a given reco energy range to compute the RMF matrix. This code is e.g. in

I think this is what you meant?

adonath commented 4 years ago

But I fully agree we should clarify and agree on the terminology...I'm adding @registerrier to the discussion as well.

maxnoe commented 4 years ago

Yes exactly, thanks @adonath

As the needed shape is known before hand, you could create the matrix using np.zeros and then assign computed columns instead of the list appending / stacking approach.