Skip to content

fix(eic): regenerate deleted compounds and samples on a mass tolerance change - #306

Merged
Jack-Coutts merged 2 commits into
mainfrom
fix/168-restored-items-get-current-eics
Nov 10, 2026
Merged

Jack-Coutts merged 2 commits into
mainfrom
fix/168-restored-items-get-current-eics

Conversation

@Jack-Coutts

Copy link
Copy Markdown
Collaborator

Fixes #168. Re-applies #214 onto main after the UI redesign.

The bug

A mass tolerance change rebuilt EICs for active compounds and active samples only. It first deleted every EIC of each active sample. A compound deleted before the change and restored after it had no EICs, so no plot and nothing to export. A sample restored after the change kept EICs built at the old tolerance.

The fix

The reload now includes soft-deleted compounds and samples, and so does the natural isotope correction pass that follows it. Restoring an item then only flips its deleted flag, as before, and its EICs are already current.

  • _iter_compounds takes include_deleted. Only the reload passes it, so the first import is unchanged.
  • regenerate_all_eics_with_mass_tolerance selects all samples and compounds, and its progress total counts them.
  • process_all_corrections corrects deleted compounds too. Without this, a restored compound's corrected EICs fall back to raw values.

Exports and the data provider already filter deleted = 0, so the extra rows stay hidden until a restore. A deleted sample whose CDF cannot be read takes the existing path from #278: it keeps its old EICs and is named in the warning.

I chose this over regenerating on restore. That would need a worker, a progress dialog and error handling in both recovery dialogs and in session import, and a restored compound would re-read every CDF.

The user guide gains two sentences under Recovering Deleted Items.

Checks

  • One new test deletes Pyruvate and sample s2, changes the tolerance from 0.2 to 0.3, and restores both through the recovery dialog. Without the change it fails with {'s2': [1.0, 1.0, 1.0]} != {'s1': [11.0, ...], 's2': [11.0, ...]}. Without only the correction change, corrected values are 11.0 instead of 12.9016. It passes with the change.
  • uv run python -m pytest -q: 672 passed.
  • bench/reference.py check: exported results unchanged.

Left out

…e change

A tolerance change rebuilt EICs for active compounds and samples only, after deleting every EIC of each active sample. A compound restored afterwards had no EICs, and a restored sample kept old-tolerance EICs. The reload and its correction pass now include soft-deleted rows, so a restore needs no extra work. Fixes #168.
@Jack-Coutts

Copy link
Copy Markdown
Collaborator Author

Review verdict for head 8310f64: CLEAN. Reviewed by GPT-6.1 Sol.

What Sol ran:

  • The full suite: 672 passed.
  • The new test with src restored from origin/main. It fails with {'s2': [1.0, 1.0, 1.0]} where both samples should be [11.0, 11.0, 11.0].
  • A database probe of deleting and restoring compounds and samples. Soft deletion flips only compounds.deleted or samples.deleted and never flags EIC, corrected or session activity rows, so rebuilt rows need no flag on restore.
  • A deleted sample with a tR override of 8.0 and offsets 0.7/1.6. The rebuild kept the override, and restoring it kept all three values.
  • A session with one deleted compound and one deleted sample, compared with main before any restore. Every workbook cell and provider value matched, and the changelogs matched apart from timestamps.

What it found:

  • No blocker or warning.
  • The reload's regenerated count and progress total now include the hidden deleted items (1 became 4 and 2/2 became 8/8 in the probe). Nothing a user exports changes.
  • A deleted sample whose CDF is gone is named in the "Regeneration Incomplete" warning on every tolerance change. That warning is the existing behaviour from fix(eic): replace EICs only after the CDF reads during a mass tolerance reload #278.

Nothing was fixed in review. main already rebuilt only active items, which is the bug this fixes.

Note for merging with #305: a deleted sample that rebuilds makes regenerated_count non-zero, so #305 keeps the new tolerance even when every active sample failed. This will be settled when the two are merged together.

…s-get-current-eics

# Conflicts:
#	src/manic/io/eic_importer.py
@Jack-Coutts
Jack-Coutts merged commit 2de1321 into main Nov 10, 2026
6 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.

Restored compounds lose their EICs after a mass tolerance change

1 participant