fix: do not write divisions in scores with no notes#337
Merged
Conversation
A source that declares no <divisions> anywhere reads as ticksPerQuarter = 0 (LCM of an empty set). The writer then emitted divisions for the first measure of every part, and core::PositiveDivisions clamped the 0 to a bogus 1e-06. The writer now omits <divisions> when the score has no time resolution; an otherwise-empty <attributes> is no longer flushed. Pins the 41 corpus files this flips to PASS (212 total).
webern
changed the title
Coverage reportCore-dev coverage
|
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 77.8% | 28514 / 36644 |
| Functions | 74.3% | 6352 / 8554 |
| Branches | 50.7% | 22680 / 44751 |
API coverage src/private/mx/{api,impl,utility}/
| Metric | Coverage | Covered / Total |
|---|---|---|
| Lines | 82.2% | 6966 / 8473 |
| Functions | 69.4% | 2322 / 3347 |
| Branches | 51.0% | 6079 / 11927 |
Core HTML report | API HTML report
Commit cbd56ddf2e0f74c6cc769b36040963682b51ee84.
gen-quality
|
3 tasks
webern
added a commit
that referenced
this pull request
Jul 12, 2026
## Human Summary It looks like the agent followed the pattern I established with tremolo marks in `MarkDataChoice` correctly and thus this follows the api standard, such as it is, and adds a couple of missing features. ## Summary Arpeggiate marks already round-tripped their direction (encoded in `MarkType::arpeggiate/arpeggiateUp/arpeggiateDown`) and shared position attributes, but dropped `number`, `unbroken` (4.0), and `id` (3.1); non-arpeggiate additionally always wrote `type="top"`, losing the bottom end of the bracket. Following the `TremoloMarkData` precedent, this adds two `MarkDataChoice` alternatives: - `ArpeggiateMarkData`: `number` (optional), `unbroken` (`Bool`), `id` (optional) - `NonArpeggiateMarkData`: `placement` (`NonArpeggiatePlacement::top/bottom`, the required MusicXML `type` attribute), `number` (optional), `id` (optional) wired through `ArpeggiateFunctions`/`NonArpeggiateFunctions` (read) and `NotationsWriter` (write). Authoring without the payload behaves exactly as before. Stacked on #337 (the arpeggiate corpus fixtures also needed the spurious-divisions fix); the 5 files this flips are pinned in `roundtrip-baseline.txt` (212 -> 217). ## Testing - [x] New `ArpeggiateAttributes` / `NonArpeggiateAttributes` round-trip tests (fail before, pass after) - [x] Full mxtest suite passes (4936+ assertions) - [x] Discovery: 217 PASS, zero regressions; regression mode: 217/217 ## References - Closes #137 - Stacked on #337 (base branch); part of #208
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
A little, innocuous bug, when fixed, unlocks our ability to run a bunch of files through
mx::api. Basically we were inserting a fictitious divisions value in scores that have no notes (and thus no need for divisions).Summary
A source that declares no
<divisions>anywhere (and has no durations) reads asScoreData::ticksPerQuarter = 0, becauseScoreReader::findMaxDivisionsPerQuartertakes theLCM of an empty set.
MeasureWriterthen unconditionally wrote divisions for the first measureof every part, and
core::PositiveDivisionsclamped the 0 to1e-06, injecting a bogus<attributes><divisions>0.000001</divisions></attributes>the source never had. This was thesingle largest api round-trip blocker (
add:divisionson 389 of 591 candidate files).The fix is a guard in
MeasureWriter: skipwriteDivisionswhen the score has no timeresolution (
ticksPerQuarter <= 0). ThePropertiesWriterbuffer then stays empty forcontent-free first measures, so no spurious
<attributes>is flushed either. Sources that dodeclare divisions — used or not — still read a positive
ticksPerQuarterand round-trip thedeclaration unchanged (e.g.
synthetic/segno.3.0.xml, which stays green).ScoreData::ticksPerQuartergets a doc comment defining the zero state, and the 41 corpusfiles this flips to PASS are pinned in
roundtrip-baseline.txt(171 -> 212).Testing
ZeroTicksPerQuarterWritesNoDivisions/ZeroTicksPerQuarterKeepsOtherPropertiesMeasureWriter tests (fail before, pass after)References
<divisions>0.000001</divisions>when the source declares no divisions #336