Skip to content

Deeper correctness & robustness fixes: known-peak indexing, unmixed-column order, interpeak split, infeasible bounds#28

Merged
gchure merged 4 commits into
mainfrom
fix/deeper-correctness
Jun 26, 2026
Merged

Deeper correctness & robustness fixes: known-peak indexing, unmixed-column order, interpeak split, infeasible bounds#28
gchure merged 4 commits into
mainfrom
fix/deeper-correctness

Conversation

@gchure

@gchure gchure commented Jun 22, 2026

Copy link
Copy Markdown
Member

Related issues: #8, #14, #15, #18, #19, #22 (the x0 is infeasible / degenerate-bounds family)

Summary

Four latent defects in Chromatogram that existing test data didn't exercise. Each fix ships with a regression test.

Fixes

I: known-peak indexing assumed the time axis starts at t = 0

scipy.signal.find_peaks returns positional indices into the current signal array, but _assign_windows mapped known_peaks to indices as np.int_(known / self._dt) — assuming t = 0 at index 0. On a chromatogram whose time axis starts at t0 ≠ 0 (or one cropped from a nonzero start), enforced peaks landed at the wrong index. The previous _crop_offset patch only worked when t0 = 0.

Fix: anchor the index math to the current frame's first time value t0 = self.df[self.time_col].values[0]. Numerically identical to the old code for the existing (uncropped, t0 ≈ 0) tests; correct for nonzero-start / cropped cases.

II: unmixed_chromatograms columns could be misaligned with peak_id

peak_df is sorted by retention_time (then peak_id = arange + 1), but the unmixed_chromatograms columns were filled in window/detection order. When a fit reorders peaks relative to detection order (e.g. a large skew shifting a retention time across a neighbor), show()'s unmixed_chromatograms[:, peak_id - 1] indexed the wrong column and mislabeled per-peak traces. (Aggregate sums elsewhere are order-independent.)

Fix: build the matrix directly from the sorted peak_df, so column i always corresponds to peak_id == i + 1. Values are identical to before; only ordering is corrected.

III: interpeak window split mishandled a gap at the first sample

_assign_windows skipped background-segment boundary construction when a background gap fell at the first sample (split_inds[0] == 0), which dropped the final background segment and mislabeled interpeak windows.

Fix: run the boundary logic for all multi-segment cases (elif split_inds[0] != 0:else:). The leading single-point run is then correctly filtered by the existing len(rng) >= 10 check.

IV: rounded location guess could fall outside its bounds → x0 is infeasible

The location initial guess is the peak time rounded to _time_precision, while its bounds are the window's raw (unrounded) time range. Because _time_precision = ceil(|log10(dt)|) is coarse for small dt, rounding could push the guess just past the window max (e.g. 0.6052 → 0.61 > 0.6052), which scipy rejects as `x0` is infeasible. This is the residual still reproducible on the issue #15 data (sandbox/raw3.csv) after the v0.2.7 bound-widening — 8 windows hit it there.

Fix:

  1. Clamp the location guess into its [lb, ub] before calling the optimizer (the true unrounded peak time is always inside the window, so this only undoes the rounding excursion). Verified: clears the x0 is infeasible crash on the ValueError: x0 is infeasible. error on fit peaks #15 data, which then fails only later with the legitimate "max function evaluations exceeded" outcome for genuinely undersampled data.

  2. Add a safety net: before fitting, validate lb < ub and lb ≤ p0 ≤ ub for every parameter and raise an actionable message naming the retention time and offending parameter — instead of scipy's opaque error. This also covers the theoretical degenerate-amplitude / single-point-window cases.

Tests

  • test_known_peaks_nonzero_start_time — fails on main, passes here.
  • test_unmixed_columns_match_peak_id — fails on main, passes here.
  • test_infeasible_location_guess_does_not_crash — fails on main with x0 is infeasible, passes here. Fixture: test_infeasible_bounds_chrom.csv (the issue ValueError: x0 is infeasible. error on fit peaks #15 chromatogram).
  • test_peak_adjacent_to_start_assigns_interpeaksmoke test. The exact split_inds[0] == 0 branch can't be triggered deterministically without coupling to scipy.peak_widths internals, so this guards the surrounding end-to-end behavior rather than isolating the branch. (A fragile internals-coupled test would be a liability; the fix itself is a provably-correct one-line change.)

…nfeasible bounds

Addresses four correctness/robustness defects in `Chromatogram`:

- E: `_assign_windows` mapped `known_peaks` to array indices assuming the time
  axis starts at t=0. Anchor the conversion to the chromatogram's first time
  value so enforced peaks land correctly on a nonzero-start or cropped trace.

- F: `fit_peaks` filled `unmixed_chromatograms` columns in window/detection
  order, which could disagree with the retention-time-sorted `peak_id` and
  mislabel per-peak traces in `show`. Build columns directly from the sorted
  peak table so column i corresponds to peak_id i+1.

- G: `_assign_windows` skipped background-segment boundary construction when a
  gap fell at the first sample (`split_inds[0] == 0`), dropping the final
  background segment. Run the boundary logic for all multi-segment cases.

- H: the location initial guess is rounded to `_time_precision` while its bounds
  are the window's raw time range; rounding could push the guess outside the
  bounds, which scipy rejects as "`x0` is infeasible" (issues #8/#14/#15/#18/
  #19/#22). Clamp the guess into its bounds, and add a safety net that raises an
  actionable message for any degenerate/inverted bound instead of scipy's
  opaque error.

Adds regression tests for E, F, and H (each fails on the prior code) plus a
smoke test for G, and a fixture derived from the issue #15 chromatogram.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.15%. Comparing base (026596d) to head (6998021).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   97.12%   97.15%   +0.03%     
==========================================
  Files           3        3              
  Lines         557      563       +6     
==========================================
+ Hits          541      547       +6     
  Misses         16       16              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gchure and others added 3 commits June 21, 2026 19:26
The two `raise ValueError` branches in the deconvolve_peaks bounds validation
(degenerate `lb >= ub` and guess-outside-bounds) were untested, dropping patch
coverage on the PR. Add a test that triggers both deterministically via
`param_bounds` (amplitude [0,0] and [2,3]x the peak value), which also exercises
the issue #22 degenerate-amplitude-bound case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion

The infeasible-location-guess crash is purely a function of `dt` versus
`_time_precision = |ceil(log10(dt))|`, not of any property of the recorded
issue #15 signal. Reproduce it synthetically instead of committing ~550
rows of third-party data of uncertain provenance/licensing:

- `dt = 0.011` gives `_time_precision = 1`, so a peak near the end of the
  record (~2.985) rounds up to 3.0, past its window's raw max — exactly the
  excursion the clamp fixes. Verified non-vacuous: with the clamp removed
  this signal raises "Initial guess for 'location' ... lies outside its
  bounds"; with it, the fit completes and recovers both peaks.

The test is now self-documenting (explains why it triggers), asserts the
`_time_precision == 1` precondition so it can't silently stop reproducing,
and runs in ~2s instead of ~7min. Deletes test_infeasible_bounds_chrom.csv.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
@gchure
gchure merged commit 6df94c1 into main Jun 26, 2026
5 checks passed
@gchure
gchure deleted the fix/deeper-correctness branch June 26, 2026 04:53
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.

1 participant