cta-observatory / pyirf

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

Fix energy_bias_resolution_from_energy_dispersion #268

Closed maxnoe closed 10 months ago

maxnoe commented 10 months ago

This function still assumed the wrong normalization of the energy dispersion fixed in pyirf 0.10.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (dbacc33) 95.35% compared to head (9a4baa1) 95.36%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #268 +/- ## ========================================== + Coverage 95.35% 95.36% +0.01% ========================================== Files 60 60 Lines 3097 3109 +12 ========================================== + Hits 2953 2965 +12 Misses 144 144 ``` | [Files](https://app.codecov.io/gh/cta-observatory/pyirf/pull/268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory) | Coverage Δ | | |---|---|---| | [pyirf/benchmarks/energy\_bias\_resolution.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvYmVuY2htYXJrcy9lbmVyZ3lfYmlhc19yZXNvbHV0aW9uLnB5) | `76.00% <100.00%> (+1.53%)` | :arrow_up: | | [pyirf/benchmarks/tests/test\_bias\_resolution.py](https://app.codecov.io/gh/cta-observatory/pyirf/pull/268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cta-observatory#diff-cHlpcmYvYmVuY2htYXJrcy90ZXN0cy90ZXN0X2JpYXNfcmVzb2x1dGlvbi5weQ==) | `71.21% <100.00%> (+4.54%)` | :arrow_up: |

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

maxnoe commented 10 months ago

@Tobychev the mistake was obvious (the function was overlooked in the update), but could you anyway maybe check with your plot in #267 ?

Tobychev commented 10 months ago

In case there is nothing in an energy bin energy_bias_resolution_from_energy_dispersion will return the upper limit of the dispersion axis. It is probably better to return NaN