cta-observatory / pyirf

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

Normalize edisp to integral of 1, not sum of 1 #250

Closed maxnoe closed 1 year ago

maxnoe commented 1 year ago

@RuneDominik does this have also implications on the interpolation code?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 99.35% and project coverage change: +0.05% :tada:

Comparison is base (19fff68) 94.73% compared to head (1164644) 94.78%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #250 +/- ## ========================================== + Coverage 94.73% 94.78% +0.05% ========================================== Files 60 60 Lines 2981 3050 +69 ========================================== + Hits 2824 2891 +67 - Misses 157 159 +2 ``` | [Files Changed](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory) | Coverage Δ | | |---|---|---| | [pyirf/interpolation/\_\_init\_\_.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9fX2luaXRfXy5weQ==) | `100.00% <ø> (ø)` | | | [pyirf/interpolation/utils.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi91dGlscy5weQ==) | `97.61% <88.88%> (-2.39%)` | :arrow_down: | | [pyirf/interpolation/component\_estimators.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9jb21wb25lbnRfZXN0aW1hdG9ycy5weQ==) | `99.27% <92.85%> (-0.73%)` | :arrow_down: | | [pyirf/interpolation/base\_extrapolators.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9iYXNlX2V4dHJhcG9sYXRvcnMucHk=) | `100.00% <100.00%> (ø)` | | | [pyirf/interpolation/base\_interpolators.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9iYXNlX2ludGVycG9sYXRvcnMucHk=) | `96.87% <100.00%> (+0.57%)` | :arrow_up: | | [pyirf/interpolation/moment\_morph\_interpolator.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9tb21lbnRfbW9ycGhfaW50ZXJwb2xhdG9yLnB5) | `100.00% <100.00%> (ø)` | | | [pyirf/interpolation/nearest\_neighbor\_searcher.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9uZWFyZXN0X25laWdoYm9yX3NlYXJjaGVyLnB5) | `100.00% <100.00%> (ø)` | | | [...yirf/interpolation/nearest\_simplex\_extrapolator.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9uZWFyZXN0X3NpbXBsZXhfZXh0cmFwb2xhdG9yLnB5) | `100.00% <100.00%> (ø)` | | | [pyirf/interpolation/quantile\_interpolator.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi9xdWFudGlsZV9pbnRlcnBvbGF0b3IucHk=) | `98.24% <100.00%> (+0.09%)` | :arrow_up: | | [...irf/interpolation/tests/test\_base\_extrapolators.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvaW50ZXJwb2xhdGlvbi90ZXN0cy90ZXN0X2Jhc2VfZXh0cmFwb2xhdG9ycy5weQ==) | `100.00% <100.00%> (ø)` | | | ... and [10 more](https://app.codecov.io/gh/cta-observatory/pyirf/pull/250?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory) | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

RuneDominik commented 1 year ago

Yes, it does. At the moment MomentMoprhInterpolator, QuantileInterpolator and MomentMorphNearestSimplexExtrapolator also norm to a sum of one. As this however only is a global factor, we could just nomalize by integration in EnergyDispersionEstimator after computing the interpolant, right?

Edit: There are probably more things that need to be changed. E.g. adding some factors to mean_std_estimation

I'm however not sure how this effects the performance of the algorithms. All EDisps I've used so far for testing summed to one as they were computed with pyirf.

maxnoe commented 1 year ago

@RuneDominik done