Skip to content
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

Wrong normalization of EDISP #249

Closed
maxnoe opened this issue Aug 1, 2023 · 7 comments
Closed

Wrong normalization of EDISP #249

maxnoe opened this issue Aug 1, 2023 · 7 comments

Comments

@maxnoe
Copy link
Member

maxnoe commented Aug 1, 2023

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
Copy link
Member Author

maxnoe commented Aug 2, 2023

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
Copy link

kosack commented Aug 2, 2023

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

@chaimain
Copy link

chaimain commented Aug 3, 2023

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
Copy link
Member Author

maxnoe commented Aug 3, 2023

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

@moralejo
Copy link
Contributor

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
Copy link
Member Author

maxnoe commented Aug 28, 2023

@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
Copy link
Member Author

maxnoe commented Aug 28, 2023

Closed by merging #250

@maxnoe maxnoe closed this as completed Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants