Update interpolation code to pyirf v0.10 API #1184
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1184 +/- ##
==========================================
- Coverage 74.42% 74.39% -0.04%
==========================================
Files 129 129
Lines 13169 13199 +30
==========================================
+ Hits 9801 9819 +18
- Misses 3368 3380 +12 ☔ View full report in Codecov by Sentry. |
| interp = GridDataInterpolator( | ||
| grid_points=grid_points, params=gh_cuts, method=method | ||
| ) | ||
| gh_cuts_interp = interp(target_point) |
There was a problem hiding this comment.
If extrapolation is needed I'd suggest to create a Estimator object (same for the AL_CUTS), just like it is done for RAD_MAX tables in pyirf. If you don't want extrapolation this should be fine.
|
@RuneDominik, ready for review. Please have a look at the changes. Although you suggested leaving all the optional arguments by default, I have included the interpolation method in the effective area and RADMAX estimators (via |
|
LGTM, if you want this to work just as the old version. One could argue that this would be a good opportunity to support the whole API, including extrapolation and support for different inter-/extrapolator classes. |
Many thanks for the quick review. I agree with you. However, this change has been pending for quite some time and I'd say it's somewhat urgent to make a new minor release (with the interpolation code at least working as before). I suggest merging this PR as it is. Make the release. And extend the support for extrapolation in another PR. What do you think, @rlopezcoto? |
|
thanks @RuneDominik for the review and the comment. I support @morcuended's point of view, since this change is urgently needed to release a bugfix, I would just stick to solve this issue. |
|
I tested these changes (using pyirf v0.10) on a Crab sample and compared them with results from lstchain v0.10.4 (pyirf v0.8 + applying the edisp fix script). Both use interpolation. The resulting SED is not the same. pyirf v0.8 + applying the edisp fix script
pyirf v0.10 (this PR)
@RuneDominik was there any change in the interpolation code of pyirf (from v0.8 to v0.10) that can explain this change in the spectrum? |
|
The was the edisp fix which was also applied to the interpolation code. As far as I see when going though the
|
Correct. I applied the script correcting the edisp normalization to the DL3 files produced with pyirf v0.8
No re-normalization was applied to the output in this case. |
|
I tested a bit and found that for data with a logarithmic binning (hence edisps) the results have changed when switching between pyirf 0.9 (pre normalization fix) and pyirf 0.10 (post normalization fix), as you can see here in the residuals between truth and estimated (and re-normalized if needed) Gaussian histograms.
For linear binning I found no difference. Thinking of it this seems plausible, as the bin-width quite drastically changes for logarithmic binning and is applied for normalization in the pyirf 0.10 version. As of now I would not call each of them better or worse, just different. Although more thorough tests are ofc. needed. |
|
Thanks a lot for the check @RuneDominik. In view of this, I think we can proceed with the merging. |
|
thanks a lot for the fix and the checks @morcuended and @RuneDominik! if you approve this, I think we can move on with this PR |
See #1160
Changed the interpolation code used in lstchain to support the new API of pyirf as explained in #1160. These changes keep the same functionality as before. They do not allow for new extrapolation functionality (that can be done elsewhere).