cta-observatory / pyirf

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

Wrong normalization of EDISP #249

Closed maxnoe closed 1 year ago

maxnoe commented 1 year ago

GADF states that EDISP needs to be a PDF (i.e. the integral should be 1), we compute a probability for each bin (i.e. the sum is 1).

I am not sure what this as for consequences e.g. for the LST analysis and how large of a difference this makes, to be checked.

CC @chaimain, @moralejo

maxnoe commented 1 year ago

Just checked the DL3 releases in the joint crab of MAGIC, VERITAS and HESS and they use the pdf definition. Will check the public CTA IRFs soon, but I doubt they'll be different.

FACT's are normalized like here, which is where we inherited that bug from.

kosack commented 1 year ago

It reminds me this needs to be written as an interface requirements for DPPS-SUSS

chaimain commented 1 year ago

It is not easy to test this with the current lstchain v0.10.x (#1141), as we need an updated MC production from lstmcpipe.

Once we have the new production, testing just the Edisp normalization on say, the LST analysis of Crab Nebula data, can be done without needing to update lstchain's dependency on pyirf (which should be done in the near future) and maybe we will see some minor difference.

maxnoe commented 1 year ago

@chaimain I am testing it now, will write an email soon. The existing dl3 files can easily be fixed without any recomputation.

moralejo commented 1 year ago

Thanks @maxnoe. Does the EDISP have units attached? (for the "z" axis) - that would make it clear it is a probability density and not a bin-integrated probability. In one of the MAGIC programs for spectral calculations we use bin-integrated probabilities, with very fine Etrue bins, and it is of course not a problem if you treat them as such in the steps afterwards.

maxnoe commented 1 year ago

@moralejo no, it does not, as the axis has no dimension

\mu = \frac{E_\text{est}}{E_\text{true}}

And it's a conventional choice if you store probabilities or probability densities, but of course it makes a difference if you use the wrong convention.

maxnoe commented 1 year ago

Closed by merging #250