Repository navigation
feat(core): time signatures in the measure table, and the shared meter examples - #736
Conversation
build_rows gives each MeasureRow the meter meters_of computes for it, with desktop#621's rules, and what follows from it: the time signature as text and as numbers (ts_numerator, exact, and ts_denominator), and the flags assumed, ambiguous and conflicting. format_time_signature writes the text: the sum of the beat values when they are all equal (6/8), the values joined by "+" as they fall in the bar when they differ (2+2+3/8), in brackets when the numerator isn't whole or holds a fraction ((4/3)/4, (1/2+1)/8). The rows of a folded table get their meters too. A row's fractions aren't JSON numbers, so the test that turns a row into JSON writes them as text. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…code shared_examples.json holds 18 beat timelines with the meters and quarter stamps that desktop#621's BeatTimeline gives them, at 07cb6d4, loaded as TiLiA loads a file. Among them are a timeline whose first measures have no beat unit, and one with no unit at all, which #621's loader gives a copy of the first unit, or the default, assumed. The expected values were computed by running #621's code, not written by hand; the time signature's text is #621's beat values written by format_time_signature, since #621 has none. test_timemap_shared.py compares build_rows' meters and time signatures with the file, and checks its layout. Comparing the quarter stamps waits for the time map's (TimeLineAnnotator/project-management#215), so that test is skipped. times_of is empty until the lookups add their cases. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f354be0 to
42e9a51
Compare
|
🤖 Claude Rebased onto |
azfoo
left a comment
There was a problem hiding this comment.
🤖 Claude
A code-review run on 42e9a514, with each finding checked by hand. The same session wrote the PR, so this review may miss what the author missed. Four points to fix; four more were checked and need no change.
To fix
- The derived fields aren't tied to
meter.time_signature,ts_numerator,ts_denominator,assumed,ambiguousandconflictingare init fields, copied frommeterinbuild_rows_from. Sodataclasses.replace(row, meter=other), or a row built by hand, can disagree with its own meter. Computing them in__post_init__, asfield(init=False), keeps them as fields, soasdictand the index still get them, and nothing can make them disagree. That is howTimeMapcomputes its arrays from its rows. test_timemap_shared.py's docstring claims too much. The beat values, flags and qstamps come from #621's code. Thetime_signaturetext is #621's values written byformat_time_signature, so the file can't check that function against itself; the test against #193's table does. The docstring should say so, as the PR description does.- Three cases depend on loading each case as a file, not two. In "a unit on a later beat of the first measure", a timeline built beat by beat gets the default unit on its first beat, so measure 1 reads 4/4, conflicting, instead of 4/8. The app's half has to load the cases as the fill script does. WP9's spec says this for "the cases without a unit in their first measures"; it should say "without a unit on their first beat", and so should TimeLineAnnotator/project-management#220 (the app's half). Nothing changes in this PR.
- Short names for the project-management issues the tests cite: #193 in
test_timemap_table.py, and #215 in the skip reason.
Checked; no change needed
json.dumps(dataclasses.asdict(row))now needsdefault=str. This is deliberate and in the description, and WP9's spec now says so. The index storests_numeratoras a real taken from theFraction, not through JSON.- The skipped
test_qstampscompares floats exactly. At a beat, #621'sget_quarter_stamp_by_timereturnsfloat(stamp)exactly, so the comparison holds if #215 does the same. #215 adapts the placeholder when it removes the skip. row.time_signature(text) androw.meter.time_signature(a tuple) share a name. Both names are fixed elsewhere: the row's is the index column's, and the meter's is #621's.- The expected values come from #621 at
07cb6d4f, nota17bbd83. This is right: #621 was rebased, soa17bbd83is no longer on its branch, and the task was updated to07cb6d4f.
time_signature, ts_numerator, ts_denominator, assumed, ambiguous and conflicting were passed to MeasureRow beside its meter, so a row made with dataclasses.replace(row, meter=...), or built by hand, could disagree with its own meter. They are now init=False fields that __post_init__ computes from the meter, as TimeMap computes its arrays from its rows: still fields, so dataclasses.asdict gives them, and replace() refuses them. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…from The beat values, flags and quarter stamps in shared_examples.json come from desktop#621's code; its time_signature text is #621's values written by format_time_signature, which test_timemap_table.py checks against texts written by hand. The docstring said every expected value came from #621. The issues the tests cite get short names. Part of TimeLineAnnotator/project-management#214 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude The four points of the review above are done:
531 passed, 18 skipped and 8 xfailed on Python 3.10 and 3.11, and |
🤖 Claude
Gives each row of the measure table its meter and time signature, and adds the shared meter examples: beat timelines with the meters and quarter stamps that #621's code (beat units and time signatures) gives them.
Builds on #732 (copy
units.pyagain), now inv1-prerelease, whose tuples let a row that holds its meter be hashed.What changed
table.py:MeasureRowgainsmeter(theMeasureMeterthatmeters_ofcomputes for it),time_signature,ts_numerator(aFraction, exact),ts_denominator,assumed,ambiguousandconflicting.build_rowsgives each row its meter, and the row computes the other six from it when it is made (init=Falsefields set in__post_init__), so they can't disagree with it;dataclasses.replacerefuses them. Folded tables get them too.format_time_signature(beat_values, denominator): the sum of the beat values when they are all equal (6/8); the values joined by+, as they fall in the bar, when they differ (2+2+3/8; three taps under2+3give2+3+2/8); in brackets when the numerator isn't whole or holds a fraction ((4/3)/4,(1/2+1)/8).shared_examples.json: 18 meter cases, laid out with one beat or one meter per line. Among them: the default unit, a pickup, 6/8 tapped in two (before and after its unit is set) and in six,2+3,1/2, a unit changing in mid-piece, a unit on a later beat, two units in one measure, an ambiguous fit, a pickup holding the end of a pattern (and one without a unit of its own), a numerator that isn't whole, no unit in the first measures, no unit at all, a unit on a later beat of the first measure, and an assumed unit in mid-piece.times_ofis empty until the lookups add their cases.test_timemap_shared.py: builds each case withdocs.pyand comparesbuild_rows' time signatures, beat values and flags with the file, and checks the file's layout and that it holds the cases the spec asks for. The quarter-stamp comparison is skipped, naming TimeLineAnnotator/project-management#215 (quarter stamps and distances), which removes the skip; its body assumesTimeMap.qstamp(t), and Make restoring state a lot faster #215 may adapt it.dataclasses.asdict(row)still works, butjson.dumpsnow needsdefault=str, which writes them as text ("3/2"), as the shared examples do. The test that turns a row into JSON does that.The core and #621 agree on every case: no case needed a difference reported.
How the expected values were computed
The beats of each case were chosen by hand. The expected meters and quarter stamps come from #621's own code at
07cb6d4f(BeatTimeline.measure_metersandquarter_stamps), never written by hand: the script below loads each case as TiLiA loads a file (create_timelinewith the components, sodeserialize_componentsand_ensure_first_beat_unitrun), and writes the values into the file. #621 has no text form of a time signature, sotime_signatureis #621's beat values and denominator written byformat_time_signature, which the tests check against the table of TimeLineAnnotator/project-management#193 (how the index shows time signatures) separately.It ran in a detached worktree of
07cb6d4f, which it left unchanged, with this branch's environment (tiliacame from that worktree, which the script asserts):fill_shared_examples.pyBecause the cases with no unit on their first beat (no unit in the first measures, no unit at all, and a unit on a later beat of the first measure) depend on
_ensure_first_beat_unit, the app's half of these examples has to load each case the same way. Building it beat by beat, withput_beat_unit_on_beatafterwards, would leave the default unit on the first beat: the first two would read it instead of a copy of the first unit, and the third would read conflicting.Tests
uv sync --all-packages, thenuv run --package tilia-core pytest packages/tilia-core -q: 531 passed, 18 skipped (the quarter-stamp comparisons) and 8 xfailed (the self-test's placeholders for later tasks), on Python 3.11.uv run --isolated --python 3.10 ...): the same counts.python scripts/check_packages.pyon Python 3.14.6: all package checks passed.Guarantees, and what removing their code did
Each was neutralised alone, the package's tests were run, and the code was restored (531 passed again). The counts are for the head of the branch,
852a027a.6/8)+2+3+2/8)ts_numeratoris exactfloatts_denominatoris the meter'sassumedis the meter'sFalseambiguousis the meter'sFalseconflictingis the meter'sFalseaa0c84edmeters_ofgivesbeat_valuesas a listWith #733
#733 (choosing the time map) also changes
table.pyandtest_timemap_table.py. This PR changesMeasureRow, the loop ofbuild_rows_fromand addsformat_time_signature, and leaves_closing_barline, the line that calls it, and__init__.pyalone. Whichever merges second rebases. Once both have landed,tilia_core.timemap.__all__should also exportformat_time_signature, andMeter(MeasureMeter, the type ofMeasureRow.meter), as the planned API lists them.Part of TimeLineAnnotator/project-management#214
🤖 Generated with Claude Code