Repository navigation
fix(eic): keep each stored tR window when the mass tolerance changes (#187) - #304
Merged
Merged
Conversation
Collaborator
Author
|
Review verdict for head 4de1cca: CLEAN. Reviewed by GPT-6.1 Sol. What Sol ran:
What it found:
Nothing was fixed in review. |
This was referenced Nov 10, 2026
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 #187.
Re-applies #217 onto
main.The bug
Changing the mass tolerance re-extracted every EIC at the default 0.2 min tR window. A window set with Apply window was lost, and so was the wider window Add Compound extracts at from a compound's tR tolerance. Plots narrowed and the tR Window field changed. A window narrower than 0.2 was widened instead.
The fix
regenerate_all_eics_with_mass_toleranceineic_importer.pynow reads each sample's stored EIC windows and re-extracts each compound at its stored window. It still widens a window that would not cover the integration offsets. A compound with no stored EIC in a sample keeps the old rule, the default window or the offset minimum. The shared window passed to the extraction is now 0.0, so it no longer widens a narrower stored window.The user guide and the mass tolerance reference say that a tolerance change keeps each EIC's tR window.
Checks
tests/test_mass_tolerance_keeps_tr_window.pydrives the window. It writes a CDF, imports it, sets Glucose's window with Apply window (0.9, and 0.15 in a second case), adds Lactate with a 0.6 tR tolerance through Add Compound, then changes the mass tolerance. It asserts each stored window and the EIC's time range. Without the fix both cases fail, for example{'Glucose': (0.2, 4.8, 5.2)} != {'Glucose': (0.9, 4.1, 5.9)}and{'Lactate': (0.2, 4.8, 5.2)} != {'Lactate': (0.6, 4.4, 5.6)}. With it, 2 passed.uv run python -m pytest -q: 673 passed.bench/reference.py check: exported results unchanged.Left out of #217
Its test in
test_rt_window_centering.py, which stubbed the CDF reader and called the function directly. The new test runs through the window in its own file. The subquery and second bind parameter in the per-sample query are replaced by a separate query for the stored windows, so this does not touch the query's WHERE clause or parameters.