Adding bounds to peak top fit in gauss_mode_width_max()#640
Adding bounds to peak top fit in gauss_mode_width_max()#640cVogl97 wants to merge 4 commits intolegend-exp:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds explicit fit-window bounds checking and constrains the peak-top Gaussian fit in gauss_mode_width_max() to improve fit stability/safety when operating near histogram edges. Also adds a debug log when bounds are applied in fit_binned().
Changes:
- Add a
ValueErrorguard when the computed fit range exceeds histogram bounds ingauss_mode_width_max(). - Add Minuit parameter bounds (
mu,sigma,a) to the peak-top fit ingauss_mode_width_max(). - Add a debug log statement when applying bounds in
fit_binned().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| guess=guess, | ||
| cost_func=cost_func, | ||
| bounds={ | ||
| "mu": (bins[i_0], bins[i_n + 1]), |
There was a problem hiding this comment.
In gauss_mode_width_max, the mu bound upper limit uses bins[i_n + 1], but the fit is performed over hist[i_0:i_n] with bin edges bins[i_0:i_n+1] (i.e. x-range [bins[i_0], bins[i_n]]). Using bins[i_n + 1] allows mu to move outside the x-range that the fit data covers; it should be consistent with the passed bins slice (e.g. cap at bins[i_n]) or adjust the indexing scheme so the bound matches the fit range.
| "mu": (bins[i_0], bins[i_n + 1]), | |
| "mu": (bins[i_0], bins[i_n]), |
| if mode_guess is not None: | ||
| i_0 = pgh.find_bin(mode_guess, bins) | ||
| else: | ||
| i_0 = np.argmax(hist) | ||
| mode_guess = bin_centers[i_0] | ||
| amp_guess = hist[i_0] | ||
| i_0 -= int(np.floor(n_bins / 2)) | ||
| i_n = i_0 + n_bins | ||
| if i_0 < 0 or i_n >= len(hist): | ||
| msg = f"Fit range exceeds histogram bounds: i_0={i_0}, i_n={i_n}, hist_len={len(hist)}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
pgh.find_bin() can return -1 (underflow) or len(bins) (overflow). In this function, amp_guess = hist[i_0] happens before the new fit-range validation, so an out-of-range mode_guess can silently pick hist[-1] or raise IndexError before you raise the intended ValueError. Consider validating i_0 immediately after find_bin() (and before indexing hist/bin_centers) and raising a clear ValueError when mode_guess is outside the histogram range.
| if i_0 < 0 or i_n >= len(hist): | ||
| msg = f"Fit range exceeds histogram bounds: i_0={i_0}, i_n={i_n}, hist_len={len(hist)}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
New behavior: this now raises ValueError when the computed fit window exceeds the histogram bounds. There is existing test coverage for gauss_mode_width_max, but no test asserting this error path. Consider adding a unit test that constructs a histogram and calls gauss_mode_width_max with mode_guess near an edge (or too-large n_bins) and asserts the ValueError (and message) to prevent regressions.
|
Can you try to understand why the tests fail and fix? |
Adds bound to the peak top fit in
gauss_mode_width_max()and includes checking that the fit range does not exceed the histogram range.Adds debug statement to
fit_binned().