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

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

Merged
merged 15 commits into from Aug 23, 2023
Merged

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Aug 2, 2023

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

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

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

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

Additional details and impacted files
@@            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 Coverage Δ
pyirf/interpolation/__init__.py 100.00% <ø> (ø)
pyirf/interpolation/utils.py 97.61% <88.88%> (-2.39%) ⬇️
pyirf/interpolation/component_estimators.py 99.27% <92.85%> (-0.73%) ⬇️
pyirf/interpolation/base_extrapolators.py 100.00% <100.00%> (ø)
pyirf/interpolation/base_interpolators.py 96.87% <100.00%> (+0.57%) ⬆️
pyirf/interpolation/moment_morph_interpolator.py 100.00% <100.00%> (ø)
pyirf/interpolation/nearest_neighbor_searcher.py 100.00% <100.00%> (ø)
...yirf/interpolation/nearest_simplex_extrapolator.py 100.00% <100.00%> (ø)
pyirf/interpolation/quantile_interpolator.py 98.24% <100.00%> (+0.09%) ⬆️
...irf/interpolation/tests/test_base_extrapolators.py 100.00% <100.00%> (ø)
... and 10 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RuneDominik
Copy link
Member

RuneDominik commented Aug 2, 2023

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.

Copy link
Member

@RuneDominik RuneDominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides some minor docstring updates and a missing test I did not find any major things here. (And ofc we need a changelog entry here, but I guess this one is obvious)

pyirf/interpolation/base_extrapolators.py Show resolved Hide resolved
pyirf/interpolation/base_interpolators.py Outdated Show resolved Hide resolved
pyirf/interpolation/tests/__init__.py Outdated Show resolved Hide resolved
@maxnoe
Copy link
Member Author

maxnoe commented Aug 22, 2023

@RuneDominik done

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

Successfully merging this pull request may close these issues.

None yet

2 participants