Low-risk correctness fixes: multi-peak param_bounds, crop(None), window index bound + test hardening#27
Merged
Merged
Conversation
Three scoped fixes plus test hardening: * deconvolve_peaks: the global `param_bounds` initial-guess check used a positive index into the per-window `p0` list, which always referred to the first peak's parameters. For windows with >1 peak this validated the wrong peak's guess. Use the existing negative-index map `paridx` so each peak is checked against its own guess; remove the now-dead `key_inds`. * crop: passing no/`None` time window raised an opaque `TypeError: object of type 'NoneType' has no len()`. Guard it with a clear ValueError. * _assign_windows: the window-range index filter used `<= len(norm_int)`, admitting one out-of-range index; use `<`. Test hardening: convert vacuous `try/except: assert True` blocks (which pass even when no exception is raised) to `pytest.raises` in test_peak_fitting, test_deconvolve_peaks, and test_crop; add regression tests for the multi-peak bound check and the crop(None) guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
=======================================
Coverage 97.12% 97.12%
=======================================
Files 3 3
Lines 556 557 +1
=======================================
+ Hits 540 541 +1
Misses 16 16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Hoist `import scipy.stats` to module level instead of inside test_multipeak_param_bounds_validated_per_peak, matching the other top-level imports in the suite. - Pin the crop(None)/crop() assertions to the guard's specific message via `match='must be provided as a list'`, so a future refactor can't satisfy them with an unrelated ValueError (e.g. the length-2 check). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
crop(None) now raises ValueError, but the docstring still claimed "If None, the entire time range ... will be considered" — behavior that was never actually implemented (the old code crashed with TypeError). Document time_window as required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gchure
added a commit
that referenced
this pull request
Jun 26, 2026
Resolves a conflict in tests/test_chromatogram.py where both branches appended new tests after test_generic_param_bounding: main (via #27) added test_multipeak_param_bounds_validated_per_peak, while this branch added the deeper-correctness regression tests. Kept both. Also dropped the now-redundant local `import scipy.stats` in the _skewnorm_signal helper, since #27 added that import at module level. quant.py merged cleanly: #27's per-peak `paridx` bound check and this branch's location clamp + pre-fit safety net coexist. Full suite: 23 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small, self-contained correctness/robustness fixes that change no behavior for valid input, plus hardening of the test suite's exception checks.
Fixes
I: multi-peak
param_boundsvalidated the wrong peak's guessIn
deconvolve_peaks, the globalparam_boundsfeasibility check indexed the per-window initial-guess listp0with a positive index (p0[key_inds[p]]), which always pointed at the first peak's parameters. For a window containing more than one peak, this validated the wrong peak's guess against the supplied bounds — risking a spuriousValueErroror a missed one. The adjacent peak-specific block already uses the correct negative-index mapparidx.Fix: use
p0[paridx[p]]so each peak is checked against its own guess; remove the now-deadkey_inds. Single-peak windows are unaffected (the indices coincide), so existingtest_generic_param_bounding/test_boundingare unchanged.II:
crop()with no/Nonewindow gave a cryptic errorcrop()/crop(None)fell through tolen(time_window)and raisedTypeError: object of type 'NoneType' has no len().Fix: add a leading guard that raises a clear
ValueErrorinstructing the caller to pass[start, end].III: off-by-one in window-range index filter
_assign_windowsfiltered candidate indices with<= len(norm_int), admitting one out-of-range index. Harmless today (the value is only used via.isin) but incorrect.Fix: use
< len(norm_int). No observable output change.Test hardening
Several exception tests used the
try/except: assert Truepattern with noassert Falseafter the call, so they passed even when no exception (or the wrong one) was raised. Converted these topytest.raises(...)intest_peak_fitting,test_deconvolve_peaks, andtest_crop, and added regression tests for the multi-peak bound check and thecrop(None)guard.Risk
Low. No behavior change for valid input; the only newly-raised error (the
crop(None)guard) replaces an existing crash with a clearer message.