Skip to content

Update Handling of Sequence Variants (Part 8): Point LoadRnaXML to new VariantApplicationMethod - #972

Closed
trishorts wants to merge 45 commits into
smith-chem-wisc:masterfrom
trishorts:rnaDbLoader_xmlUpdateOne
Closed

Update Handling of Sequence Variants (Part 8): Point LoadRnaXML to new VariantApplicationMethod#972
trishorts wants to merge 45 commits into
smith-chem-wisc:masterfrom
trishorts:rnaDbLoader_xmlUpdateOne

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

current LoadRnaXml has two int variables: maxVariantsForCombinatorics =4 and minAlleleDepth = 1. VariantApplication now has three consensusPlusVariantIsoforms = 1, minAlleleDepth = 0 and maxVariantsPerIsoform = 0. This PR changes LoadRnaXML to call use all three variables. It also calls the new GetConsensusAndVariantBioPolymers method in VariantApplication

this may break some tests in MM where variants are expected but none are called for. The former LoadXML called for 4 with a min depth of 1. defaults now are zero.

@codecov

codecov Bot commented Oct 26, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.30075% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.91%. Comparing base (57f1327) to head (57cdb48).
⚠️ Report is 92 commits behind head on master.

Files with missing lines Patch % Lines
mzLib/Omics/BioPolymer/SequenceVariation.cs 56.75% 22 Missing and 10 partials ⚠️
mzLib/Omics/BioPolymer/VariantApplication.cs 65.43% 20 Missing and 8 partials ⚠️
mzLib/UsefulProteomicsDatabases/ProteinXmlEntry.cs 88.54% 7 Missing and 4 partials ⚠️
...Databases/DecoyGeneration/DecoyProteinGenerator.cs 12.50% 0 Missing and 7 partials ⚠️
...micsDatabases/DecoyGeneration/RnaDecoyGenerator.cs 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
- Coverage   81.00%   80.91%   -0.10%     
==========================================
  Files         269      269              
  Lines       38826    38957     +131     
  Branches     4241     4272      +31     
==========================================
+ Hits        31450    31521      +71     
- Misses       6640     6678      +38     
- Partials      736      758      +22     
Files with missing lines Coverage Δ
mzLib/Omics/BioPolymer/VariantCallFormat.cs 86.95% <ø> (ø)
mzLib/UsefulProteomicsDatabases/ProteinDbLoader.cs 96.03% <100.00%> (ø)
mzLib/UsefulProteomicsDatabases/ProteinDbWriter.cs 95.48% <100.00%> (ø)
...ProteomicsDatabases/Transcriptomics/RnaDbLoader.cs 93.69% <100.00%> (ø)
...micsDatabases/DecoyGeneration/RnaDecoyGenerator.cs 83.72% <50.00%> (ø)
...Databases/DecoyGeneration/DecoyProteinGenerator.cs 90.79% <12.50%> (ø)
mzLib/UsefulProteomicsDatabases/ProteinXmlEntry.cs 96.78% <88.54%> (-2.75%) ⬇️
mzLib/Omics/BioPolymer/VariantApplication.cs 82.76% <65.43%> (+0.99%) ⬆️
mzLib/Omics/BioPolymer/SequenceVariation.cs 66.93% <56.75%> (-19.03%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@acesnik

acesnik commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

🤖 The delta against #970 is one commit doing to LoadRnaXML what #970 did to LoadProteinXML, so everything in my comment on #970 applies verbatim: maxVariantsPerIsoform is still never read by GetConsensusAndVariantBioPolymers, and the defaults still move 4 → 1 and 1 → 0.

The positional hazard is materially worse here, though, and it is worth separating from the #970 version because the failure mode is different.

The new parameter lands immediately before int maxThreads

-            int maxHeterozygousVariants = 4, int minAlleleDepth = 1,
+            int consensusPlusVariantIsoforms = 1, int minAlleleDepth = 0, int maxVariantsPerIsoform = 0,
             int maxThreads = 1, IHasChemicalFormula? fivePrimeTerm = null, ...

In #970 the parameter after the insertion point was bool addTruncations, so a positional caller that reached it failed to compile — int will not bind to bool. Here the neighbour is another int, so a positional caller silently shifts its thread count into maxVariantsPerIsoform — a parameter that does nothing — and maxThreads falls back to 1. Four consecutive int parameters, all defaulted, is a shape that cannot fail loudly.

And it has already happened once, on this exact method

MetaMorpheus/EngineLayer/DatabaseLoading/DatabaseLoadingEngine.cs:151 on MetaMorpheus master:

rnaList = RnaDbLoader.LoadRnaXML(fileName, generateTargets, decoyType, isContaminant,
    GlobalVariables.AllRnaModsKnown, modTypesToExclude, out unknownMods,
    commonParameters.MaxThreadsToUsePerFile, decoyIdentifier: decoyIdentifier);

That is eight positional arguments. On master's signature, parameter 8 is maxHeterozygousVariants, not maxThreadsmaxThreads is parameter 10. So MaxThreadsToUsePerFile is being passed as the heterozygous-variant combinatorics limit, and maxThreads stays at its default of 1.

The reason is visible in the same file: LoadProteinXML puts maxThreads at position 8, before the variant parameters, and the protein call at :180 binds correctly. The RNA call was written as though LoadRnaXML had the same order. It does not.

Two consequences on MetaMorpheus master today, independent of this PR:

  • RNA XML decoy generation runs with maxThreads = 1 whatever the user configured.
  • The variant combinatorics limit is the machine's thread count. So how deeply variants expand depends on how many threads the run was given rather than on the data — the same class of machine-dependent irreproducibility as Build PEP training rows in a fixed order, so match-between-runs is reproducible #1155. Note the protein side deliberately passes 0 there.

There is a // TODO: Add in variant params when fixed in MzLib. immediately above that call, so the variant parameters were known to be unwired — but the actual defect is the thread count landing in one of them.

I will raise that separately since it is a MetaMorpheus bug rather than yours. It is directly relevant here because it is evidence that this parameter block is already being mis-bound positionally, which is the strongest argument for appending maxVariantsPerIsoform after isEntrapment rather than inserting it mid-list — or keeping the old signature as a delegating wrapper, the way GetVariantBioPolymers does at VariantApplication.cs:48-52.

Same recommendation as the rest of the series

Re-cut against current master. This branch's base is 2025-10-24 and master's LoadRnaXML has since gained entrapmentIdentifier and isEntrapment, so the signature this PR edits is not the signature that exists — which is also why the argument positions above need rechecking after any rebase rather than being taken from this diff.

@acesnik

acesnik commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

🤖 Closing alongside #969 and #970, which this was stacked on, in favour of #1222.

Same reasoning as #970 — the loader rewiring is a separate decision from the validity predicate and #1222 leaves it out — with one addition specific to this branch: inserting maxVariantsPerIsoform immediately before int maxThreads means a positional caller silently loses its thread count into a parameter that does nothing, rather than failing to compile as it would on the protein side.

That is not hypothetical, and reviewing this branch is how it turned up: DatabaseLoadingEngine.cs:151 in MetaMorpheus was already passing MaxThreadsToUsePerFile into maxHeterozygousVariants, because LoadRnaXML and LoadProteinXML order those parameters differently. RNA decoy generation was running single-threaded and variant expansion depth was set by the machine's thread count. Filed as smith-chem-wisc/MetaMorpheus#2762 and fixed in smith-chem-wisc/MetaMorpheus#2763 — so this branch found a real bug even though it is not landing.

Please reopen if you would rather carry it forward. If the rewiring does get re-cut, appending the new parameter after isEntrapment rather than inserting it mid-signature would avoid repeating that class of mistake.

Thanks for the work.

@acesnik acesnik closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants