From 41043772dd7b3c0c4a6b80f5a0e7eb484acbe543 Mon Sep 17 00:00:00 2001 From: gchure Date: Wed, 17 Jun 2026 23:20:19 -0700 Subject: [PATCH 1/3] Fix multi-peak param_bounds check, crop(None) guard, window index bound 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 --- hplc/quant.py | 15 ++++++---- tests/test_chromatogram.py | 58 +++++++++++++++++++++++++++----------- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/hplc/quant.py b/hplc/quant.py index 49d0941..b842333 100644 --- a/hplc/quant.py +++ b/hplc/quant.py @@ -147,6 +147,9 @@ def crop( You are trying to crop a chromatogram after it has been fit. Make sure that you do this before calling `fit_peaks()` or provide the argument `time_window` to the `fit_peaks()`.""" ) + if time_window is None: + raise ValueError( + "`time_window` must be provided as a list of [start, end].") if len(time_window) != 2: raise ValueError( f"`time_window` must be of length 2 (corresponding to start and end points). Provided list is of length {len(time_window)}." @@ -324,7 +327,7 @@ def _assign_windows( ranges = [] for l, r in zip(_left, _right): _range = np.arange(int(l - buffer), int(r + buffer), 1) - _range = _range[(_range >= 0) & (_range <= len(norm_int))] + _range = _range[(_range >= 0) & (_range < len(norm_int))] ranges.append(_range) # Identiy subset ranges and remove @@ -644,7 +647,6 @@ def deconvolve_peaks( "skew": [-np.inf, np.inf], } # Modify the parameter bounds given arguments - key_inds = {k: i for i, k in enumerate(_param_bounds.keys())} if len(param_bounds) != 0: for p in parorder: if p in param_bounds.keys(): @@ -657,13 +659,16 @@ def deconvolve_peaks( v["location"][i] + p for p in param_bounds[p] ] else: - if (p0[key_inds[p]] >= param_bounds[p][0]) & ( - p0[key_inds[p]] <= param_bounds[p][1] + # `paridx` indexes from the end of `p0`, which + # accumulates 4 entries per peak, so it always + # refers to the peak currently being set up. + if (p0[paridx[p]] >= param_bounds[p][0]) & ( + p0[paridx[p]] <= param_bounds[p][1] ): _param_bounds[p] = param_bounds[p] else: raise ValueError( - f"Bounds for parameter '{p}' [{param_bounds[p]}] is exclusive of initial guess {p0[key_inds[p]]:0.3f} for peak at retention time {p0[1]}." + f"Bounds for parameter '{p}' [{param_bounds[p]}] is exclusive of initial guess {p0[paridx[p]]:0.3f} for peak at retention time {p0[paridx['location']]}." ) # Add peak-specific bounds if provided diff --git a/tests/test_chromatogram.py b/tests/test_chromatogram.py index 8720bfe..8c27c43 100644 --- a/tests/test_chromatogram.py +++ b/tests/test_chromatogram.py @@ -66,14 +66,10 @@ def test_peak_fitting(): chrom_df = pd.read_csv('./tests/test_data/test_fitting_chrom.csv') chrom = hplc.quant.Chromatogram( chrom_df, cols={'time': 'x', 'signal': 'y'}) - try: + with pytest.raises(ValueError): chrom._assign_windows(rel_height=-1) - except ValueError: - assert True - try: + with pytest.raises(ValueError): chrom._assign_windows(rel_height=2) - except ValueError: - assert True chrom_df = pd.read_csv('./tests/test_data/test_fitting_chrom.csv') chrom = hplc.quant.Chromatogram(chrom_df[chrom_df['iter'] == 1], cols={ @@ -204,16 +200,15 @@ def test_crop(): """ data = pd.read_csv('./tests/test_data/test_assessment_chrom.csv') chrom = hplc.quant.Chromatogram(data, cols={'time': 'x', 'signal': 'y'}) - try: + with pytest.raises(ValueError): chrom.crop([1, 2, 3]) - assert False - except ValueError: - assert True - try: + with pytest.raises(RuntimeError): chrom.crop([2, 1]) - assert False - except RuntimeError: - assert True + # A missing/None time window should give a clear ValueError, not a TypeError. + with pytest.raises(ValueError): + chrom.crop(None) + with pytest.raises(ValueError): + chrom.crop() # Test that a dataframe is returned only if specified. no_returned_df = chrom.crop([10, 20], return_df=False) @@ -246,10 +241,8 @@ def test_deconvolve_peaks(): """ data = pd.read_csv('./tests/test_data/test_assessment_chrom.csv') chrom = hplc.quant.Chromatogram(data, cols={'time': 'x', 'signal': 'y'}) - try: + with pytest.raises(RuntimeError): chrom.deconvolve_peaks() - except RuntimeError: - assert True def test_map_peaks(): @@ -515,3 +508,34 @@ def test_generic_param_bounding(): assert False except ValueError: assert True + + +def test_multipeak_param_bounds_validated_per_peak(): + """ + Regression test for bug B: when a global `param_bounds` is applied to a + window containing more than one peak, the initial-guess-vs-bounds check must + use *each* peak's own guess. Previously it always inspected the first peak's + guess, so a bound that excluded a later peak's guess was silently accepted + (and later surfaced as an opaque scipy error rather than the informative one). + """ + import scipy.stats + + # Two overlapping peaks with clearly different widths that share one window. + t = np.arange(0, 30, 0.01) + sig = (200 * scipy.stats.norm(14.0, 0.2).pdf(t) + + 200 * scipy.stats.norm(15.0, 0.6).pdf(t)) + df = pd.DataFrame({'time': t, 'signal': sig}) + + # Inspect the per-peak scale initial guesses (scale guess = width / 2). + probe = hplc.quant.Chromatogram(df) + probe._assign_windows(buffer=200) + multi = [v for v in probe.window_props.values() if v['num_peaks'] == 2] + assert len(multi) == 1, "expected the two peaks to share a single window" + guesses = sorted(w / 2 for w in multi[0]['width']) + # Bound contains the smaller guess (first peak) but excludes the larger one. + bound = [0, 0.5 * (guesses[0] + guesses[1])] + + chrom = hplc.quant.Chromatogram(df) + with pytest.raises(ValueError, match='exclusive of initial guess'): + chrom.fit_peaks(correct_baseline=False, buffer=200, + param_bounds={'scale': bound}, verbose=False) From 59b09b9fd1b3d8666f2f923cc6a8049da5b9e740 Mon Sep 17 00:00:00 2001 From: gchure Date: Sun, 21 Jun 2026 20:37:03 -0700 Subject: [PATCH 2/3] Address review nits in param-bounds test hardening - 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 --- tests/test_chromatogram.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_chromatogram.py b/tests/test_chromatogram.py index 8c27c43..d915539 100644 --- a/tests/test_chromatogram.py +++ b/tests/test_chromatogram.py @@ -4,6 +4,7 @@ import numpy as np import matplotlib.pyplot as plt import pytest +import scipy.stats def compare(a, b, tol): @@ -205,9 +206,9 @@ def test_crop(): with pytest.raises(RuntimeError): chrom.crop([2, 1]) # A missing/None time window should give a clear ValueError, not a TypeError. - with pytest.raises(ValueError): + with pytest.raises(ValueError, match='must be provided as a list'): chrom.crop(None) - with pytest.raises(ValueError): + with pytest.raises(ValueError, match='must be provided as a list'): chrom.crop() # Test that a dataframe is returned only if specified. @@ -518,8 +519,6 @@ def test_multipeak_param_bounds_validated_per_peak(): guess, so a bound that excluded a later peak's guess was silently accepted (and later surfaced as an opaque scipy error rather than the informative one). """ - import scipy.stats - # Two overlapping peaks with clearly different widths that share one window. t = np.arange(0, 30, 0.01) sig = (200 * scipy.stats.norm(14.0, 0.2).pdf(t) From 74ff25dee87df4e09f8c275a1b4409f2bbcc0517 Mon Sep 17 00:00:00 2001 From: gchure Date: Sun, 21 Jun 2026 20:54:38 -0700 Subject: [PATCH 3/3] Align crop() docstring with the new None guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- hplc/quant.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hplc/quant.py b/hplc/quant.py index b842333..cf06fb9 100644 --- a/hplc/quant.py +++ b/hplc/quant.py @@ -130,9 +130,10 @@ def crop( Parameters ---------- - time_window : `list` [start, end], optional - The retention time window of the chromatogram to consider for analysis. - If None, the entire time range of the chromatogram will be considered. + time_window : `list` [start, end] + The retention time window of the chromatogram to consider for + analysis. This is required; a `ValueError` is raised if it is not + provided. return_df : `bool` If `True`, the cropped DataFrame is