Skip to content

fix(eic): mark samples left at an old mass tolerance in the export and plots - #310

Merged
Jack-Coutts merged 3 commits into
mainfrom
fix/206-flag-stale-samples-v2
Nov 10, 2026
Merged

Jack-Coutts merged 3 commits into
mainfrom
fix/206-flag-stale-samples-v2

Conversation

@Jack-Coutts

@Jack-Coutts Jack-Coutts commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #206. Re-applies #215 onto fix/168-restored-items-get-current-eics (#306) after the UI redesign.
Part of #307. This PR covers the marking half: a reload that stops partway leaves every sample it did not rebuild marked stale. #305 covers the message half, the failure dialog and the tolerance it puts back.

The bug

When the mass tolerance changes and a sample's CDF cannot be read, that sample keeps its EICs from the old tolerance. The "Regeneration Incomplete" warning names it once. After that dialog closes nothing shows the sample is stale, and its areas go into the workbook next to samples built at the new tolerance with no mark. With #306 deleted samples are rebuilt too, so a deleted sample with a bad CDF is stale as well and becomes stale and visible when restored.

A reload can also raise partway, for example a PermissionError while checking a later sample's CDF path (#307). The samples it already rebuilt hold the new tolerance and the rest keep the old one, with no record of which are which.

The fix

The reload records the result per sample in a new samples.eics_stale column. Before the sample loop it sets the flag on every sample, deleted ones included, in one UPDATE. A sample rebuilt at the new tolerance is set back to 0 in the same transaction that replaces its EICs. So a sample whose CDF fails, and every sample a raising reload never reached, stays marked. A reload that succeeds clears every flag, including samples whose batch is empty because every compound's RT window fell outside the scan range. list_stale_samples() in sample_reader.py is the one query for the stale state, and #244 can build its "rebuild stale samples" action on it. It includes deleted samples, so a restored stale sample is marked with no restore-specific code.

The flag shows in three places:

  • The export warning. Before the save dialog, if any active sample is stale, export names those samples and asks the user to confirm. Cancel stops the export and nothing is written. This covers what colour alone misses. The unlabelled Qualifier QC sheet shows a stale sample with its usual Pass or Fail fill, and a script reading the values never sees colour. The check reuses MainWindow._confirm, the same prompt the missing internal standard uses.
  • The exported workbook. Every cell of a stale sample is light gold on every results sheet. This reuses the peak verdict fills through a new PeakVerdict.STALE, so no sheet writer changed. Gold replaces any validation fill for that sample, because its numbers are wrong whatever the review said. The changelog's Cell Colour Key gains one line for gold, so the colour is explained next to the workbook.
  • The plot captions. A stale sample's tile caption reads s2 · old mass tolerance. The reload already re-plots when it finishes, and a restore re-plots too, so the mark appears without changes to the reload handlers.

The sample list and a separate changelog section were left out. The sample list would need a refresh hook in the reload completion handler, which #305 owns. A changelog list of names repeats what the gold cells and the export warning already show.

The flag clears when the sample is rebuilt at the current tolerance. A later successful reload clears it. A re-import clears it too, because loading CDFs goes through clear_database, which deletes the sample rows.

Older databases get the column from _run_migrations, the same ALTER TABLE ... ADD COLUMN pattern the compounds columns use, with default 0. Older samples are treated as not stale. MANIC clears the database at every start, so an older database file never holds EICs that need a flag. Session files hold methods only and need a re-import of the raw data, so they carry no stale field and gain none. A session import that changes the tolerance runs the same reload and sets the flag the same way.

The docs say what the export warning, the gold cells and the caption mean and how to clear them.

Checks

  • tests/test_stale_sample_marking.py has three tests.
    • The first reloads at 0.3 with one sample's CDF moved away and exports. It fails without the change ({'s2': {'00000000'}} != {'s2': {'FFFFE699'}}). Then it puts the file back, reloads again, and checks the gold is gone. With only the clearing write removed, that half fails ({'s2': {'FFFFE699'}} != {'s2': {'00000000'}}).
    • The second reloads four samples from 0.2 to 0.3. s1 rebuilds, s2's CDF is missing, and checking s3's path raises PermissionError, which stops the reload. It checks list_stale_samples() == ['s2', 's3', 's4']. On the previous head (feee390), which marked only after the loop, it fails with [] == ['s2', 's3', 's4']. The test patches Path.exists for s3 instead of making the folder unreadable, because chmod does not block a folder on the Windows CI runner.
    • The third opens a database whose samples table has no eics_stale column. Without the migration it fails with no such column: eics_stale. With it, no sample is stale.
  • tests/test_export_warns_about_stale_samples.py drives MainWindow.export_data with one stale sample. Declined, the user is never asked for a location and no file is written. Without the warning it fails ([PosixPath('.../out.xlsx')] == []). Accepted, the workbook is written with s1 and s2 on Raw Values.
  • A throwaway script deleted a sample, moved its CDF, reloaded at 0.3 and restored the sample. Its caption read s2 · old mass tolerance, and Raw Values, Corrected Values, Isotope Ratio, % Label Incorporation and Abundances each filled only s2 gold.
  • uv run python -m pytest -q: 677 passed.
  • bench/reference.py check: exported results unchanged.

Left out of #215

  • The warning icon and tooltip in the sample list. Its refresh call sits in _mass_tolerance_reload_completed, which fix(ui): a failed mass tolerance reload puts the old tolerance back #305 owns.
  • The changelog's "Stale Samples" section.
  • The gold fill on the unlabelled Qualifier QC sheet. That sheet colours each row by its qualifier outcome. The export warning names the stale samples before that sheet is written.
  • Its tests of tooltip and caption wording.

Known limits

  • The flag records that a sample missed the last change, not the tolerance its EICs were built at. Changing back to the tolerance a stale sample was built at, while its CDF is still missing, keeps the mark although the EICs now match. The mark errs towards a false warning, never towards hidden stale data.
  • A per-compound rebuild (a wider integration window, or a session re-extraction) rebuilds one compound at the current tolerance but leaves the sample marked.
  • While a reload runs, a sample not yet rebuilt is already marked. A plot drawn mid-reload shows the caption on it, which is true at that moment.

…d plots

A sample whose CDF could not be read during a mass tolerance reload kept its old-tolerance EICs. After the warning closed nothing showed it, and its areas went into the workbook unmarked. The reload now records the failure in samples.eics_stale and clears it when the sample is rebuilt. The export fills that sample's cells light gold on every sheet, the changelog colour key explains the colour, and its plot captions say "old mass tolerance". Fixes #206.
…g them

Every sample is marked before the reload loop and cleared when its EICs are
replaced, so a reload that raises partway leaves unreached samples marked.
Export now asks for confirmation when an active sample is stale.
@Jack-Coutts

Copy link
Copy Markdown
Collaborator Author

Review verdict for head 3e3270d: CLEAN after one fix round. Reviewed by GPT-6.1 Sol.

Round 1, at feee390:

  • Blocker: the stale flags were written only after the reload's sample loop. Sol reproduced it with four samples at 0.2 Da reloaded at 0.3: s1 rebuilt, s2's CDF was missing, and a PermissionError on s3's path stopped the loop before s4. s2, s3 and s4 kept their 0.2 Da EICs and none of them was flagged (A mass tolerance reload that fails partway leaves samples at mixed tolerances with no mark #307).
  • Warning: colour alone let stale data look usable. The unlabelled Qualifier QC sheet showed a stale sample with a green Pass, and a script reading the values never saw the colour.
  • What Sol checked and found fine: every PeakVerdict consumer (no KeyError, no miscount, STALE is never saved as a review), labelled and unlabelled exports with validation on and off, the migration, clear_database, and session round trips. Full suite: 674 passed. Results check: no changed cells.

Fix, feee390..3e3270d:

  • One UPDATE marks every sample stale before the loop, and each sample is cleared in the transaction that replaces its EICs.
  • Export now asks for confirmation and names the stale active samples. Cancel writes nothing.

Round 2, the fix diff only:

  • Sol's repro now flags s2, s3 and s4. The new test fails against feee390 with [] instead of ['s2', 's3', 's4'].
  • Flags clear for deleted samples, empty tR batches, zero matching masses and empty target lists.
  • With no compounds or no samples, the reload raises before anything is marked.
  • The export check fires for a hidden active stale sample and ignores deleted ones. Declining reaches neither the save dialog nor a file write.
  • Full suite: 677 passed.

Defect already on main, outside this pull request: when the corrections step fails after a reload, the error is only logged, after the old corrected rows were deleted. A direct DataExporter export then writes blank corrected cells. Export from the window retries the corrections first and writes no workbook if they still fail.

@Jack-Coutts
Jack-Coutts changed the base branch from fix/168-restored-items-get-current-eics to main November 10, 2026 20:26
…mples-v2

# Conflicts:
#	docs/01_user_guide.md
#	docs/Reference_Mass_Tolerance.md
@Jack-Coutts
Jack-Coutts merged commit f434af1 into main Nov 10, 2026
10 checks passed
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.

Samples whose CDF is missing keep old-tolerance EICs in the export

1 participant