Repository navigation
fix(integration): re-centre every sample's EIC when Apply changes the tR - #302
Merged
Merged
Conversation
Apply and session import now re-extract a sample unless extracting its tR Window around its tR would return the stored trace unchanged and that window covers both offsets. This replaces the coverage test with 0.001 min of slack and the per-sample bounds cache, which stored bounds under the wrong sample when one had no EIC.
Conflicts: - docs/01_user_guide.md: main's session-import step and backward-compatibility text, with this branch's sentence on when an import re-extracts. - integration_window_widget.py: keep main's _stored_tr_windows (#298); drop check_boundaries_within_window, which this branch replaces with eic_needs_reextraction. - test_rt_window_centering.py: regeneration now refreshes only the tR Window field, plots and charts (no bounds refresh here, no fields refresh after #289). test_session_apply_refresh.py (#289) edited tR on fake CDF paths and relied on Apply not re-extracting. With this branch a tR change re-extracts, the read fails, and the error box blocks the test. Its two tests now edit the left offset inside the tR Window on EICs stored centred on the tR, which keeps Apply on the no-re-extraction path #289 guards.
…EIC was cut around Apply with a tR Window range re-extracted every sample at the widest stored window. Each re-extracted sample now keeps its own window, raised only to cover its own offsets. Session import does the same. The re-extraction check no longer guesses from the stored scans. Each EIC row stores the tR it was extracted at (eic.extraction_rt, added by a migration; NULL in older databases means re-extract). A sample is re-extracted unless that tR equals its new tR and its stored window covers both offsets. This fixes irregular scans that skipped a needed re-extraction and runs clipped at the start or end that re-extracted on every Apply.
Collaborator
Author
|
Review verdict for head a64ae13: CLEAN after one fix round. Reviewed by GPT-6.1 Sol. Before the review, Round 1, at e5bd61f:
Round 2, the fix diff only:
Merge note: #304 and #308 edit the same EIC inserts. When resolving, keep the sixth batch value (the extraction tR) and the six-placeholder INSERT, so the mass tolerance reload records |
This was referenced Nov 10, 2026
…-on-tr-change # Conflicts: # docs/01_user_guide.md
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.
Fixes #264. Fixes #188.
Re-applies #266 onto
main, with the session-import half of #267.The bug
After a tR change, plots of one compound could be centred on different tRs, even when every sample had the same tR. Apply re-extracted a sample only when the new integration window fell outside its stored EIC, with 0.001 min of slack. A tR change inside the stored data left the plot where it was. Sub-second differences in each file's first scan decided which samples moved. Session import used the same test, so an imported session showed the same split.
The test read each sample's data range from a cache in the widget. That cache paired sample names with EICs by position, so one sample with no EIC shifted every later sample's bounds onto the wrong name (#188).
The fix
Each
eicrow now stores the tR it was extracted at, in a newextraction_rtcolumn. Import, Apply, the tR Window update and the mass tolerance reload all write it.One check in
eic_importer.py,eic_needs_reextraction, is used by Apply and byreextract_session_eics. A sample is re-extracted unless its storedextraction_rtequals its new tR and its stored tR Window covers both offsets. A sample with no EIC is always re-extracted.regenerate_compound_eicstakes a window per sample, as it already took a tR per sample. Session import does the same. With one value in the field, nothing changes.Why store the tR instead of reading it from the stored scans: an earlier version of this branch judged "centred" from the first and last stored scan and the median scan interval. With irregular scans that skipped a needed re-extraction, and a trace clipped by the start or end of the run failed the check on every Apply. Comparing the stored tR is exact and needs no tolerance. Both values come from the same parsed field or the same database value, so they compare equal when the tR has not changed.
The tR Window itself is not compared. Apply never changes the window unless an offset outgrows it, and the tR Window update re-extracts every sample on its own.
Older databases get the column through
_run_migrations. Their EICs have no stored tR, so the first Apply on each compound re-extracts them once.The per-sample bounds cache, its refresh after regeneration,
calculate_integration_boundariesandcheck_boundaries_within_windoware deleted. This closes #188.Unlike #266, the old "Regeneration Complete" box is not touched:
mainalready shows a toast.Cost
One Apply that moves Lactate's tR by 0.02 min on the bench's 30 labelled samples: 2.0 ms on
main(re-centres 0 of 30) and 70 ms here (re-centres 30 of 30). Median of 5 alternating runs per side, range 67 to 112 ms.read_cdf_filetakes about two thirds of it. The machine was heavily loaded during the runs, so treat the range as wide. These were measured before the check stopped decompressing stored traces, which only makes the check cheaper.Results
bench/reference.py checkpasses. The reference never changes a tR, so it cannot see this change.Exported areas after a tR change do move. With deconvolution on, the fit uses the whole stored trace, so the area depends on where the trace was cut. I applied tR moves to Pyruvate, Lactate, Glutamate and Valine on the bench data and exported from
mainand from this branch. Pyruvate moved by about 0.01%. Lactate moved by a median of 0.14% and up to 5%. Two Glutamate samples moved by up to 17% in raw values. Valine did not move. I then set those four tRs in the compound list, extracted fresh, and exported. That export matches this branch in every area. Only the header row that prints the compound-list tR differs. So after this change, a tR set with Apply gives the same areas as the same tR in the compound list. Onmainit depended on edit history.Checks
tests/test_apply_recentres_eics.pydrives the realIntegrationWindowon real CDF files.assert [(4.8, 5.2), (4.8, 5.2)] == [(4.9, 5.3), (4.9, 5.3)], so the plots stay on the old tR. With it, both re-centre, a narrower offset re-extracts nothing, and an offset past the window re-extracts.{'S1': 0.4} != {'S1': 0.2}.[['S1', 'S2', 'S3']] == [['S1', 'S3']].[] == ['S1'].The session import test has a sample nudged inside its stored data. Without the fix only
Movedis re-extracted. With it,MovedandNudgedare, each at its own window, andKeptis not.uv run python -m pytest -q: 652 passed.eictable frommain's schema gainsextraction_rton start, and its rows are marked for re-extraction.Left out
_populate_range_fieldsstill reads every EIC only to check that one exists. Left alone to stay in scope.