Adapted quantile interpolation #174
Conversation
|
Is there any update on this or #163? |
There was a problem hiding this comment.
A general comment, also for the other PR:
Please don't implement the actual interpolation logic inside interpolate_energy_dispersion.
Implement functions interpolate_binned_pdf_<method>(binned_pdfs, edges, grid_points, interpolation_point, **options) (That should be the required arguments, right?)
This function should then be just applied along the relevant axes in interpolate_energy_dispersion and interpolate_psf which could have a method= argument if we decide to merge both approaches.
We cannot make the assumption of equal distance bins, at least not in general. We could make them in the interpolation method if that is strictly required, but then interpolate_energy_dispersion should check for equal-distance logspace binning in migra and then do the interpolation in logspace.
Also please split up the steps inside into single functions. E.g. the resampling of the interpolated pdf back to the original bin edges.
* Renamed hists-variable to binned_pdf * Array broadcasting instead of repeating * Isolated interpolation algorithm into function * Segmented interpolation algorithm into subfunctions * Handled a problem where cdfs could become constant and inverting would raise an error * Typos
Minor changes: * Removed two comments * Renamed reconstruct_pdf_values to pdf_from_ppf * Astropy units in A_eff interpolation defaults * Reverted one change introduced by black
* Fixed a problem where pdfs that started with a bin != 0 couse problems due to a cdf consequently starting with a value != 0 by introducing a dummy bin with content 0 in front of every pdf. This bin is most often ignored due to the selective ppf computation. * Moved bin_mids computation inside ppf_values-function as they are only needed there
* Assuming correct GADF IRFs so the axis keyword is no longer needed * Reworked Doc-Strings
* Deleted tests for interpolate_energy_dispersion and interpolate_psf_table * Added tests for the interpolation code itself using linearly changing Gaussians on a 1D Grid
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
==========================================
- Coverage 91.10% 90.73% -0.37%
==========================================
Files 41 41
Lines 1607 1598 -9
==========================================
- Hits 1464 1450 -14
- Misses 143 148 +5
Continue to review full report at Codecov.
|
|
The |
pyirf/interpolation.py
Outdated
|
|
||
| # Interpolate pdf samples and evaluate at bin edges, weight with the bin_width to estimate | ||
| # correct bin height via the midpoint rule formulation of the trapezoidal rule | ||
| pdf_values = np.apply_along_axis( |
There was a problem hiding this comment.
I assume this was black? This looks horrible...
Maybe better just define a normal function using def in here instead of using a lambda?
This is an alternative interpretation to the approach taken in #163. Instead of following the steps of this tech report closely, @maxnoe suggested to implement different solutions to the steps summarized in the tech reports Fig. 5.
On the upside, this method should scale easily to nD interpolation variables without using Delaunay simplices. This comes, however, at the price of worse results, at least in the testing case in my comment in #161.