Repository navigation
fix(integration): re-centre every sample's EIC when Apply changes the tR - #266
Closed
Jack-Coutts wants to merge 3 commits into
Closed
Jack-Coutts wants to merge 3 commits into
Jack-Coutts wants to merge 3 commits into
Conversation
… applied tR Replace the coverage-check tests with one rule test: a stored EIC is re-extracted when it is missing, more than one scan from its tR, or narrower than the offsets. Apply that re-extracts now finishes without a message box.
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 centred on the old tR, and sub-second differences in each file's first scan decided which samples moved. Samples with one tR could show different x axes. samples_needing_recut reads each sample's stored EIC and its tR Window from the database. Apply re-extracts every sample that is missing, more than one scan from its tR, or narrower than the offsets. This deletes the per-sample bounds cache that the coverage check used. Apply now finishes silently after a re-extraction, as it does when nothing is re-extracted. Fixes #264 Fixes #188
…sets A trace clipped at the start or end of the run, or one with a large scan gap, could pass the centring check while the integration window ran past its data. The rule now also requires the stored data to cover tR minus the left offset to tR plus the right offset, and measures one scan with the median interval. The stored tR Window check is redundant with the coverage check and is removed.
Collaborator
Author
|
Replaced by #302. |
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.
Why
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. A user report showed 15 of 25 Sucrose samples re-centred and 10 left on the old tR.
What changed
samples_needing_recutreads each sample's stored EIC from the database. Apply re-extracts every sample whose EIC is missing, is more than one scan from its tR, or does not cover tR minus the left offset to tR plus the right offset.Scope
Tradeoffs
Re-extracting on every tR change costs about 30 ms per 107 MB sample once #265 lands, and about 340 ms without it. Extracting a wider margin and only re-centring on large moves would keep small edits instant, but it keeps the coverage check this PR removes.
Blast Radius
Every tR change made with Apply now re-reads the CDF files for the samples it applies to. Compounds near the start or end of a run re-extract on every Apply, because their traces are clipped. The results are identical.
Verification
uv run pytest: 513 passed. The new tests cover a trace off-centre but covering the offsets, a trace narrower than the offsets, a missing trace, a trace clipped at the run start, and a trace with a scan gap.MainWindowoffscreen on the 30-sample labelled bench. Plotting Pyruvate and applying tR 6.93 (left offset 0.035, right offset 0.05) to all tiles: onmain, all 30 tiles stay centred on 7.088. On this branch, all 30 are centred on 6.929, with no message box. With tR unchanged, nothing is re-extracted.Fixes #264
Fixes #188