Skip to content

fix: narrow an oversized octave shift to the closest ottava line - #423

Merged
webern merged 2 commits into
mainfrom
claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size
Aug 29, 2026
Merged

fix: narrow an oversized octave shift to the closest ottava line#423
webern merged 2 commits into
mainfrom
claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size

Conversation

@webern

@webern webern commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Human Summary

Some corpus files existed with bad octave-shift size values. lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml was apparently created for this purpose. synthetic/octave-shift.3.0.xml and synthetic/octave-shift.3.1.xml were created by an LLM that might not have known better (just going off of the schema).

Either way, the right thing to do is to test the expected behavior of mx which is to coerce these to the next lower, valid, values of 22, 15 or 8.

Summary

MusicXML lets octave-shift/@size be any positive integer, and three corpus files use that room for a number that names no line a performer could read: 27, 11 and 1 (lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml is named for it). mx::api narrows each of those to an ottava it can draw, so the round-trip saw a 27 go in and a different number come out.

Narrowing is the behavior we want — mx::api models the six ottava lines music notation has, and everything downstream is allowed to assume an ottava is one of them. Two changes, one to the rule and one to the round-trip.

The rule now picks the closest line. DirectionReader::parseOctaveShift matched 22 exactly, so a source asking for 27 fell through to the 15th line even though the 22nd was both available and closer to what it asked for. It matches 22 and up instead. Sizes of exactly 8, 15 and 22 are unaffected, so this only moves sources that were already asking for something notation has no name for.

The round-trip expects the narrowed size. narrowOctaveShiftSizes() in CorpusRoundtripMain.cpp rewrites octave-shift/@size on the expected document by that same rule, rather than the api learning to echo the original number back out. That is the move the harness already makes for mx's attribution stamp: add it to the expected side instead of stripping it from the output.

  • Only the expected side is narrowed, so a write that produced a size other than 8, 15 or 22 still fails.
  • A stop's size is taken there from the stop's own attribute, while mx takes it from the start the stop closes. A source whose stop contradicts its start still fails, which is what OttavaStopSizeContradictionIsNormalizedToTheStart covers from the api side.
  • The api already round-tripped the attribute's presenceOttavaStart::writeDefaultSize and OttavaStop::writeSize from feat: derive octave-shift stop size from its start #415 — so only the value ever differed. No public api change.

Round-trip corpus

Adds 3 files to roundtrip-baseline.txt (407 -> 410). value:octave-shift@size was the sixth entry on the classifier worklist.

Testing

Three new OttavaSizeApiTest cases state the narrowing away from the corpus: a size between the lines taking the 15th, a size past the 22nd taking the 22nd, and a size below 8 becoming a plain octave line.

The gates below were run against the first commit. The second commit (the >= 22 rule) landed after, at the author's direction, and is covered by CI on this PR.

  • make api-test (6348 assertions in 588 test cases)
  • make api-roundtrip (410 passed, 0 failed of 410 pinned)
  • make api-roundtrip-discover: the PASS set is exactly the 410 pinned files — the 3 new passes and no regressions
  • make core-roundtrip-test (839 test cases) and make core-unit (221 assertions in 44 test cases)
  • make fmt and make fmt-check

References

@webern webern added feature new feature request breaking fixes or implementation that require breaking changes api Affects the mx::api layer impl Affects the mx::impl layer ai Issues opened by, or through, a coding agent. labels Aug 23, 2026 — with Claude
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size branch from c04b36d to d3d8b60 Compare August 23, 2026 19:29
@webern webern changed the title feat: keep an octave-shift size the ottava type does not imply test: narrow the expected octave-shift size in the api round-trip Aug 23, 2026
@webern webern added testing and removed feature new feature request breaking fixes or implementation that require breaking changes api Affects the mx::api layer impl Affects the mx::impl layer labels Aug 23, 2026 — with Claude
@webern webern changed the title test: narrow the expected octave-shift size in the api round-trip fix: narrow an oversized octave shift to the closest ottava line Aug 23, 2026
@webern webern added bug software defect impl Affects the mx::impl layer labels Aug 23, 2026 — with Claude
@webern webern removed the bug software defect label Aug 23, 2026
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-05-voice-label branch from f27752f to e8c81aa Compare August 26, 2026 20:41
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size branch from 33002fc to f73ead1 Compare August 26, 2026 20:42
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-05-voice-label branch from e8c81aa to 7a2c4ce Compare August 28, 2026 20:54
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size branch from f73ead1 to ac496f4 Compare August 28, 2026 21:01
Base automatically changed from claude/mx-api-roundtrip-corpus-9pkrg6-05-voice-label to main August 28, 2026 21:31
webern added 2 commits August 28, 2026 21:33
MusicXML lets octave-shift/@SiZe be any positive integer, and three corpus files
use that room for a number that names no line a performer could read: 27, 11 and
1. mx::api narrows each to the nearest ottava it can draw, so the round-trip saw
a 27 go in and a 15 come out.

The narrowing is the behavior we want. mx::api models the six ottava lines music
notation has, and everything downstream is allowed to assume an ottava is one of
them. So the harness brings the expected document to the size mx::api writes,
the way it already adds mx's attribution stamp to the expected side rather than
stripping it from the output.

Only the expected side is narrowed, so a write that produced a size other than
8, 15 or 22 still fails. Two api tests state the narrowing on its own, away from
the corpus.

Pins the three files in the api round-trip baseline.
The size narrowing matched 22 exactly, so a source asking for 27 fell through
to the 15th line even though the 22nd was both available and closer to what it
asked for. Match 22 and up instead.

lysuite/ly33e_Spanners_OctaveShifts_InvalidSize.xml states size 27, and now
round-trips as 22 rather than 15. The round-trip harness narrows the expected
document by the same rule.
@webern
webern force-pushed the claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size branch from ac496f4 to 7c3ca4c Compare August 28, 2026 21:33
@webern
webern merged commit 5973bfa into main Aug 29, 2026
8 checks passed
@webern
webern deleted the claude/mx-api-roundtrip-corpus-9pkrg6-06-octave-shift-size branch August 29, 2026 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai Issues opened by, or through, a coding agent. impl Affects the mx::impl layer testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant