Skip to content

Carry the cleavage-blocking flag through a file-specific rebuild - #24

Draft
trishorts wants to merge 3 commits into
masterfrom
fix/file-specific-cleavage-blocking-flag
Draft

trishorts wants to merge 3 commits into
masterfrom
fix/file-specific-cleavage-blocking-flag

Conversation

@trishorts

@trishorts trishorts commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Draft, parked on the fork. This was smith-chem-wisc#2845, moved here while it is a draft to keep the upstream PR list uncluttered. Its earlier review threads and their answers stay on the upstream PR: smith-chem-wisc#2845. It goes back upstream as a live PR when it is ready.

Base: master -- needs an mzLib release carrying smith-chem-wisc/mzLib#1119's RespectCleavageBlockingModifications before it can build.

Project of origin: cleavageRules

SetAllFileSpecificCommonParams rebuilds a task's DigestionParams from an explicit list of fields whenever a spectra file has its own settings. Its own comment says what happens to a field left off that list:

Every digestion setting a file does not override must be carried from the task. A field left out of this list silently falls back to its constructor default for every file with file-specific settings; the glycopeptide and SILAC flags used to be (see FileSpecificDigestionParamsTests).

mzLib's DigestionParams.RespectCleavageBlockingModifications is left off it. Its default is false, so any file with any file-specific setting — even one that only changes a precursor tolerance — gets the historical modification-blind digest while the task reports the corrected one. The flag also reaches IndexingEngine.ToString(), so the two runs are not distinguishable from the output afterwards.

One argument closes it:

respectCleavageBlockingModifications: digestionParams.RespectCleavageBlockingModifications

The guard that was supposed to catch this, and why it didn't

FileSpecificDigestionParamsTests was written for exactly this class of bug — the glycopeptide and SILAC flags were dropped the same way — and its second test compares the whole object with DigestionParams.Equals, whose remarks promise that "a digestion setting added in the future that is forgotten in the rebuild fails here too."

It did not fail. A whole-object comparison only guards a field the fixture sets away from its constructor default, because the rebuild's fallback is the constructor default, so a field left at its default agrees by accident. The fixture now sets the flag true, which makes that guard real, and the remarks say so in as many words for whoever adds the next field.

Tests

  • FileSpecificRebuild_WithoutDigestionOverrides_KeepsEveryDigestionSetting — fixture extended, so the existing whole-object guard actually fires.
  • FileSpecificRebuild_KeepsRespectCleavageBlockingModifications(true|false) — new, so the failure report names the setting that was dropped rather than printing two long strings.

Mutation-checked. With the one argument removed and everything else unchanged, both the true case and the whole-object test fail — the latter with 4,Cleave,6,40,256,3,trypsin,Semi,C,False,True,True,True became 4,Cleave,6,40,256,3,trypsin,Semi,C,False,True,True,False. The false case passes throughout, so the set cannot pass or fail vacuously.

How this was run, and what it is waiting on

Draft. It needs the mzLib change that adds the flag, which is mzLib smith-chem-wisc#1119; MetaMorpheus's mzLib PackageReference has to move past that release before this compiles on CI.

Locally it was built against mzLib packed from smith-chem-wisc#1119 at its current head, plus mzLib smith-chem-wisc#1330 — without smith-chem-wisc#1330 MetaMorpheus master does not compile at all against any mzLib that carries smith-chem-wisc#1318's abstract IDigestionParams.SpecificDigestionAgent. With that package: MetaMorpheus builds with 0 errors, the targeted sweep over FileSpecificDigestionParams|Toml|DigestionAgentName|ParameterTest is 61 passed / 0 failed, and the full MetaMorpheus suite is 2313 passed / 0 failed / 1 skipped.

That full-suite number is worth one more sentence, because it is the first time MetaMorpheus's tests have run against this mzLib at all: master's integration job has been unable to compile MetaMorpheus since mzLib smith-chem-wisc#1318 merged, so nothing has exercised the pair. It also required mzLib smith-chem-wisc#1318's toml regression to be fixed, which MetaMorpheus smith-chem-wisc#2835 does — this build carries that PR's two SpecificDigestionAgent IgnoreProperty calls but deliberately not its .IgnoreProperty(p => p.SpecificProtease), and all 2314 tests pass without it. That is the measurement behind the finding I left on smith-chem-wisc#2835.

Un-draft and re-run once smith-chem-wisc#1119 merges and ships.

🤖 Generated with Claude Code

trishorts and others added 3 commits September 21, 2026 17:17
SetAllFileSpecificCommonParams rebuilds DigestionParams from an explicit list
of fields whenever a spectra file has its own settings, and its own comment
warns that a field left out of that list "silently falls back to its
constructor default for every file with file-specific settings". mzLib's
RespectCleavageBlockingModifications was left out, and its default is false,
so any file with any file-specific setting -- even one that only changes a
mass tolerance -- got the historical modification-blind digest while the task
reported the corrected one. The flag also feeds the index cache fingerprint,
so the two runs are not distinguishable afterwards.

FileSpecificDigestionParamsTests already claimed to guard against exactly this
by comparing the whole object with DigestionParams.Equals, and it did not
fire: the rebuild's fallback IS the constructor default, so a field the
fixture leaves at its default agrees by accident. The fixture now sets the
flag true, which makes that guard real, and the remarks say so for the next
field. A named test covers both states so the failure report says which
setting was dropped.

Requires the mzLib change that adds the flag (mzLib smith-chem-wisc#1119); draft until that
merges and ships.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clean merge; no conflicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-cleavage-blocking-flag

# Conflicts:
#	MetaMorpheus/Test/FileSpecificDigestionParamsTests.cs
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.

1 participant