Skip to content

Fix ProForma writer for ranges bearing multiple modifications (§4.5)#116

Merged
rfellers merged 3 commits into
topdownproteomics:masterfrom
trishorts:fix/proforma-writer-multi-mod-range
Jun 30, 2026
Merged

Fix ProForma writer for ranges bearing multiple modifications (§4.5)#116
rfellers merged 3 commits into
topdownproteomics:masterfrom
trishorts:fix/proforma-writer-multi-mod-range

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

Problem

ProFormaWriter.WriteString throws ProFormaParseException: "Can't nest ranges within each other." whenever a term contains a sequence range that carries more than one modification — the ProForma 2.0 §4.5 construct (SEQ)[mod1][mod2]…. The parser accepts these strings, so they cannot currently be round-tripped: parsing succeeds but writing the resulting term fails.

Reproduce

var parser = new ProFormaParser();
var writer = new ProFormaWriter();
var term = parser.ParseString("PRT(ESFRMS)[Oxidation][Oxidation][half cystine][half cystine]ISK");
writer.WriteString(term);   // throws: Can't nest ranges within each other.

This also blocks real specification examples (e.g. the §4.5 insulin-style range bearing multiple oxidations and half-cystines).

Root cause

Each modification on a range is parsed as a separate ProFormaTag spanning the same (ZeroBasedStartIndex, ZeroBasedEndIndex) — the parser keeps the range open across consecutive )[mod][mod] until the next residue resets it. When the writer reaches the range, its inner loop scans the tags that fall within the range and unconditionally throws for any whose start != end. But a co-located range modification legitimately has start != end (it spans the whole range), so these were mistaken for nested ranges.

Fix

In the range branch of WriteString, a tag whose start and end equal the current range's start and end is treated as another modification on that same range and emitted as a consecutive [descriptor] after the range closes. Genuinely nested ranges (a different start != end inside the range) still throw as before.

After

PRT(ESFRMS)[Oxidation][Oxidation][half cystine][half cystine]ISK   // round-trips unchanged
SE(QUEN)[+14.05][Oxidation]CE                                       // two modifications on one range

Tests

  • WriteMultipleModificationsOnSameRange — constructs a range carrying two modifications and asserts SE(QUEN)[+14.05][Oxidation]CE.
  • RoundTripMultipleModificationsOnRange — parse→write round-trip of a four-modification range.
  • Full suite green: 235 passed, 7 skipped, 0 failed (net472 + net6.0).

🤖 Generated with Claude Code

https://claude.ai/code/session_01JNnpzBNwfnTbZxtZdtAzLG

The writer threw "Can't nest ranges within each other" for a range carrying more
than one modification (e.g. "(SEQ)[mod1][mod2]", ProForma 2.0 section 4.5). Each
modification on a range is parsed as a separate tag spanning the same start/end,
and the writer's internal-tag loop mistook those co-located modifications for
nested ranges.

Treat a tag whose start and end match the current range as another modification
on that range and emit it as a consecutive descriptor after the range closes;
genuine nested ranges still throw. Adds a constructed unit test and a parse/write
round-trip regression test; full suite green (235 passed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNnpzBNwfnTbZxtZdtAzLG
@trishorts trishorts requested a review from rfellers June 19, 2026 16:09
@trishorts trishorts added the bug Something isn't working label Jun 19, 2026
rfellers
rfellers previously approved these changes Jun 30, 2026

@rfellers rfellers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks!

trishorts pushed a commit to trishorts/sdk that referenced this pull request Jun 30, 2026
The Build and Test job failed on all open PRs (topdownproteomics#116, topdownproteomics#117, topdownproteomics#118) because
the final Codecov step errored on upload (exit 4294967295) while
fail_ci_if_error was true, marking the whole job failed even though
restore, build, and tests all passed (235 passed, 0 failed).

- fail_ci_if_error: true -> false so a Codecov-side hiccup no longer
  fails the build
- fix coverage file path: coverage.net6.0.xml -> coverage.xml to match
  the Coverlet output (/p:CoverletOutput='../../coverage.xml')
- bump checkout/setup-dotnet/codecov-action v3 -> v4 to clear the
  Node 20 deprecation warnings

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.43%. Comparing base (b4d1949) to head (291403e).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   89.23%   89.43%   +0.20%     
==========================================
  Files         103      105       +2     
  Lines        5721     5851     +130     
  Branches      882      912      +30     
==========================================
+ Hits         5105     5233     +128     
- Misses        429      430       +1     
- Partials      187      188       +1     
Files with missing lines Coverage Δ
src/TopDownProteomics/ProForma/ProFormaWriter.cs 93.29% <100.00%> (+4.24%) ⬆️

... and 4 files with indirect coverage changes

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

Adds two ProFormaWriterTests exercising the branches introduced by the
multi-mod-range fix that were previously uncovered (codecov/patch):
- WriteSingleResidueModificationInsideRange: a point modification localized
  to one residue inside a range emits within the parentheses.
- WriteNestedRangesThrows: a genuine nested range (distinct start/end) is
  still rejected with ProFormaParseException.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rfellers rfellers merged commit e031543 into topdownproteomics:master Jun 30, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants