Skip to content

fix(glyco): name the missing GraphCheck precondition instead of throwing NullReference - #2699

Merged
trishorts merged 1 commit into
smith-chem-wisc:masterfrom
trishorts:fix/glyco-localizegraph-precondition
Aug 4, 2026
Merged

fix(glyco): name the missing GraphCheck precondition instead of throwing NullReference#2699
trishorts merged 1 commit into
smith-chem-wisc:masterfrom
trishorts:fix/glyco-localizegraph-precondition

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

The problem

LocalizationGraph.LocalizeOGlycan finishes by reading the graph's terminal node:

https://github.com/smith-chem-wisc/MetaMorpheus/blob/3b9f634ec/MetaMorpheus/EngineLayer/GlycoSearch/LocalizationGraph.cs#L140

localizationGraph.TotalScore = localizationGraph.array[modPos.Count - 1][localizationGraph.ChildModBoxes.Length - 1].maxCost + noLocalScore;

That node is only populated when the peptide's candidate-site motifs can actually accommodate every modification in the box. GlycoSearchEngine guarantees it by screening with GraphCheck before building the graph, so in a search this is always safe and this PR changes nothing.

It is not safe when the graph is driven directly. A peptide offering one candidate site against a two-glycan box leaves the terminal node null, and the symptom is a bare NullReferenceException pointing at an array index — nothing that names the missing precondition:

System.NullReferenceException : Object reference not set to an instance of an object.
   at EngineLayer.GlycoSearch.LocalizationGraph.LocalizeOGlycan(...) in LocalizationGraph.cs:line 140

I hit this writing a harness that builds localization graphs outside the engine, and it cost real time to trace back to the unstated GraphCheck requirement. LocalizationGraph is otherwise pleasantly standalone — its unit tests already construct graphs by hand — so directly-driven use is a reasonable thing to do and likely to recur.

The change

Read the terminal node once, and if it is null throw MetaMorpheusException naming the box size, the candidate-site count, and the precondition:

Localization graph has no reachable terminal node for a modification box of 2 modification(s) over 1 candidate site(s). The candidate-site motifs cannot accommodate the box; callers must screen with GlycoSearchEngine.GraphCheck before localizing.

Twelve lines including the comment, all after the scoring work. No behavioural change on any input that reaches this line today.

MetaMorpheusException is the repo's fault type here rather than an ArgumentException because it is what EngineCrashed already reports on for engine-layer faults.

Test

OGlycoTest_LocalizeOGlycan_TooFewSitesForBox_ThrowsDescriptive — one candidate site against a two-glycan box, asserting MetaMorpheusException and that the message names GraphCheck. Fails with NullReferenceException before this change.

Verification

Against master at 3b9f634ec, --filter "FullyQualifiedName~Glyco":

Passed Failed
master 86 5
this branch 87 5

Same five failures, which are pre-existing on master and unrelated — they are the shared-state ordering bug fixed by #2698.

Alternative considered

Returning a zero TotalScore instead of throwing. Rejected: an unreachable terminal node means the caller asked for something structurally impossible, and silently scoring it zero would let a mis-specified graph flow into results looking merely unconvincing rather than invalid.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xa43sN3mQ96uLmpSNcWj9h

…erence

LocalizeOGlycan ends by reading the graph terminal node, which is only
populated when the peptide candidate-site motifs can accommodate every
modification in the box. GlycoSearchEngine guarantees that by screening with
GraphCheck first, so a search never hits this and behaviour is unchanged.

Driving the graph directly without that precondition -- from a test, or from
new calling code -- leaves the node null and produces a bare
NullReferenceException on an array index, naming nothing. LocalizationGraph is
otherwise standalone, and its own unit tests build graphs by hand, so this is a
reasonable thing to do and likely to recur.

Read the node once and throw MetaMorpheusException naming the box size, the
candidate-site count and the precondition.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xa43sN3mQ96uLmpSNcWj9h
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (c037f64) to head (cb2605a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2699   +/-   ##
=======================================
  Coverage   93.28%   93.29%           
=======================================
  Files         214      214           
  Lines       21760    21804   +44     
  Branches     4073     4080    +7     
=======================================
+ Hits        20298    20341   +43     
  Misses        910      910           
- Partials      552      553    +1     
Flag Coverage Δ
unittests 93.29% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pheus/EngineLayer/GlycoSearch/LocalizationGraph.cs 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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

@trishorts
trishorts requested a review from pcruzparri August 4, 2026 20:49
@trishorts
trishorts merged commit 75040ce into smith-chem-wisc:master Aug 4, 2026
11 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.

4 participants