Repository navigation
feat(core): the measure table's structure and the time map's positions - #707
Conversation
azfoo
left a comment
There was a problem hiding this comment.
🤖 Claude
This review comes from a code-review run plus a read-through. I reproduced bugs 1–4 and 8 on 49a05c2; the rest come from reading the code. Please fix them before it merges.
Bugs
-
A short span, or a point with float noise, can end the bar before it starts (
timemap.py:80). A point is recognised only whenend == startexactly, and the end is snapped on its own. On a 4/4 map:positions(4.0, 4.05)gives bar 2,end_bar1;positions(4.0, 4.0 + 1e-12)gives bar 2,end_bar1.
The unit then ends before it starts, and a query like
bar = 2..2misses it. Compare the snapped end with the snapped start instead: when the end isn't after the start,end_barisbar. -
A short unit at the first beat is said to run past the end (
timemap.py:118)._end_barreturnsNonewhenever the snapped end is at or before the map's start. Sopositions(0.0, 0.05)gives bar 1 andend_barNone, which is what a unit running past the closing barline gets (FR-032). The fix for 1 covers this too. -
Whether a time 0.1 before a beat snaps depends on float rounding (
timemap.py:100). 3.9, 0.1 before bar 2, isn't snapped: bar 1, beat 4.9. 7.9, 0.1 before bar 3, is snapped: bar 3, beat 1. Compare withtolerance + EPS. -
Rows and time maps can't be pickled or copied (
table.py:114).MeasureRow.metadatais aMappingProxyType, so these raiseTypeError:pickle.dumps(time_map);copy.deepcopy(row);dataclasses.asdict(row).
WP12's index may build tables in worker processes, which need pickling. Use a tuple of pairs, or a read-only mapping type that pickles.
-
Beats out of time order give a wrong map, with no reason (
timemap.py:139). Only two neighbouring beats at exactly the same time are refused:- Beats at 0, 1, 3 and 2 give an
endof 1.0, before the beat at 3.0, andpositions(1.5, 1.5)isNone. - Two beats a rounding error apart, 1.0 and 1.0 + 1e-9, pass. Bar 1 then has three beats, and every beat number after them in that bar shifts.
Refuse times that don't rise by more than
EPS. Give the second case the existing reason, "two beats at the same time (1.0)", and the first "beats out of time order (at 2.0)". - Beats at 0, 1, 3 and 2 give an
-
"Fewer than two beats" for a timeline that has many (
timemap.py:137). A timeline with no measure table (measures=None) gets this reason, and so does one whose rowsrows_ofall skipped. WP3's reader doesn't build the table yet (TimeLineAnnotator/project-management#181), so every real file gets this reason today. Give it its own reason: "no measure table". -
A skipped measure is absorbed by the one before it (
table.py:66).countis the index inrows_of's filtered list. In WP3's table, though, a row's index + 1 is itsbar.count. With measures at 0, 2 and 4, where the downbeat at 2 has no time:- bar 3 gets count 2;
- bar 1 ends at 4.0;
positions(2.5, 2.5)gives bar 1, beat 2.5.
Keep WP3's
count, and give no time map while a downbeat has no time, with the reason "a downbeat without a time". -
A restart on the first bar makes the first movement 2 (
table.py:59). Rows 1 (restart), 2, 1 (restart), 2 give movements 2, 2, 3, 3. That follows FR-012 as written, but a file that marks every movement's first bar, the first one included, then has no movement 1. A restart on the first row should leave the movement at 1. FR-012 is being corrected to say so.
Waits on a decision
find_rows ignores the movement (table.py:140). With a restart, find_rows(rows, 1, pass_=1) gives bar 1 of both songs. That question is TimeLineAnnotator/project-management#252 (which bar a number and a pass name). It doesn't block this PR: leave find_rows as FR-061 has it for now.
Smaller
build_time_mapworks outfoldedagain (timemap.py:142), thoughbuild_rowsalready gives it asMeasureRow.folded(table.py:50). Testrows[0].folded, so the rule is in one place.TimeMapholdsstart,endandtoleranceas fields (timemap.py:50), but they always equal_slots[0],_slots[-1]andTOLERANCE. Make them properties, so that adataclasses.replacecan't make them disagree with_slots. Its generated__eq__and__hash__also walk every row and beat on every call. Useeq=Falseunless something needs to compare time maps.- The test helper
tests/timemap/docs.pymakes beats withkind="BEAT", but the core's name is"beat", andcanonical_bytesrefuses"BEAT". The tests pass because nothing reads a beat's kind yet. Any test that writes a document would fail.
|
🤖 Claude Points 1–11 are fixed in five commits, with the tests written first: each new test failed before its fix. Bugs
Waits on a decision
Smaller
|
FelipeDefensor
left a comment
There was a problem hiding this comment.
🤖 Claude
Felipe approves this PR. Before merge: tests for seven claims, and the rest of the guarantees table.
Two reviews, run independently, found no wrong bar, beat, pass or movement. One compared positions with a brute-force version on 88,200 random queries: no differences. The other worked through a piece with a pickup, endings, a cadenza and a restart by hand. The suite passes on Python 3.10, 3.12 and 3.14: 288 tests, and 371 merged with #706. The eleven points of Anne's review are fixed as described.
For TimeLineAnnotator/project-management#214. This PR promises that rows hash, and test_timemap_table.py:233 checks it. That task adds meter to MeasureRow, and #706's MeasureMeter holds lists, so hash(row) will raise. Felipe suggests that TimeLineAnnotator/project-management#247 lands first, and says why there.
Before merge
-
Seven claims in the description have no test. With the code removed, all 288 tests still pass:
Claim Code A test that would catch it downbeattests the unsnapped starttimemap.py:110 Beats at 4.0 s (a downbeat) and 4.15 s: a point at 4.09 s is bar 2, beat 2, with downbeattrueAn end before the start ends in the start's bar timemap.py:98 ( <=)4/4 with a beat every second: positions(4.0, 3.0).end_baris 2. With==there, it's 1A cadenza doesn't count towards its number's passes table.py:97 Bars 1, 2, 2c, 2: the last one has pass 2, not 3 time_map_forgives no map for a timeline that isn't a beat timeline__init__.py:15A marker timeline with a measure table. The test's marker timeline has none, so it gets no map anyway Time maps compare by identity timemap.py:45, eq=FalseTwo builds of one timeline aren't equal Positionsis frozentimemap.py:29 Setting barraisesFrozenInstanceErrorThe package exports MeasureRow,PositionsandTimeMap__init__.py:6-9Import them from tilia_core.timemapThe first is P3 on TimeLineAnnotator/project-management#104, still open: such a point is "beat 2" and "on the downbeat" at once. Whichever way P3 goes, a test should pin the rule.
-
The table leaves out guarantees that the tests do catch. It has 22 rows, and the description claims more. Some, with the tests that fail without their code:
- passes count per printed number: 11 (table.py:98);
- a restart starts every count again: 4 (table.py:94), and starts the next movement: 4 (table.py:95);
foldedis set on every row: 3 (table.py:109);- the end point is excluded from
end_bar: 3 (timemap.py:143); - a time snaps to the nearer of two beats: 1 (timemap.py:126).
Principle V wants each one with its count. All 101 we checked are below.
Can follow later
- A start in the last 0.1 s before the closing barline has no position, while the same time as an end is in the last bar. test_timemap_positions.py:195 pins it. When the media caps the barline, that can be up to half of the last beat: with the last beat at 7.0 s and media of 7.3 s, a marker at 7.2 s has no bar. It follows FR-031, FR-032 and the prototype. It's Anne's call, and it needs a line in the spec either way.
- A beat
timethat isn't a number makesbuild_time_mapraiseTypeErrorinstead of giving a reason. No file reaches it today, since WP3's reader is a stub. It depends on what TimeLineAnnotator/project-management#181's reader guarantees. If it doesn't,rows_ofcould treat such a time as missing. FrozenMetadatadoesn't equal a document dict that holds lists.FrozenMetadata({"tags": ["a", "b"]}) == {"tags": ["a", "b"]}is False, though the reply above (point 4) says it compares equal to a plain dict with the same items. Say so in the docstring, or compare with the lists turned into tuples.- A file without media. If WP3 gives it a
media_lengthof 0.0 rather than None, every such file logs "the last beat (…) is at or after the end of the media (0.0)". - WP9's data-model.md:50 types
beat_idsaslist[str]. The code's tuple is right, as Anne noted on TimeLineAnnotator/project-management#211. Change the data model, not the code.
Reproduction
From the repository root at 34cee4c, with Python 3.12: PYTHONPATH=packages/tilia-core/src:packages/tilia-core/tests/timemap python repro.py
import dataclasses
import logging
from docs import beat_timeline, document
from tilia_core.timemap import time_map_for
from tilia_core.timemap.table import FrozenMetadata, build_rows
from tilia_core.timemap.timemap import build_time_map
records = []
handler = logging.Handler()
handler.emit = records.append
logging.getLogger("tilia_core.timemap").addHandler(handler)
def timeline_of(times, downbeats, marks=None):
"""Beats at `times`; those at `downbeats` start a measure, with `marks` {time: mark}."""
marks = marks or {}
return beat_timeline(
[
{"time": t, "measure": marks.get(t, {}) if t in downbeats else None}
for t in times
]
)
def time_map(times, downbeats, media_length=None):
timeline = timeline_of(times, downbeats)
doc = document(timeline, media_length=media_length)
found, reason = build_time_map(doc, timeline)
assert found is not None, reason
return found
def where(p):
return None if p is None else f"bar {p.bar}, beat {p.beat:g}, end_bar {p.end_bar}, downbeat {p.downbeat}"
four_four = time_map([float(t) for t in range(12)], {0.0, 4.0, 8.0})
# Before merge, 1: downbeat tests the unsnapped start (G23).
close = time_map([0.0, 1.0, 2.0, 3.0, 4.0, 4.15, 5.0, 6.0, 7.0], {0.0, 4.0})
for t in (4.09, 4.12):
print(f"1. beats at 4.0 (a downbeat) and 4.15, a point at {t}: {where(close.positions(t, t))}")
# Before merge, 1: time_map_for's kind check (G55), on a timeline that isn't a beat timeline but has a measure table.
timeline = timeline_of([float(t) for t in range(8)], {0.0, 4.0})
marker = dataclasses.replace(timeline, kind="marker")
doc = document(marker)
print("1. time_map_for, a marker timeline with a measure table:", time_map_for(doc, marker.id))
print("1. build_time_map on the same timeline gives a map:", build_time_map(doc, marker)[0] is not None)
# Before merge, 1: an end before the start ends in the start's bar (G17).
print("1. 4/4, positions(4.0, 3.0):", where(four_four.positions(4.0, 3.0)))
# Before merge, 1: a cadenza doesn't count towards its number's passes (G64), bars 1, 2, 2c, 2.
marks = {0.0: {"number": 1}, 4.0: {"number": 2}, 8.0: {"number": 2, "label": "2c", "cadenza": True}, 12.0: {"number": 2}}
timeline = timeline_of([float(t) for t in range(16)], set(marks), marks)
print("1. bars 1, 2, 2c, 2, passes:", [(row.label, row.pass_) for row in build_rows(document(timeline), timeline)])
# Before merge, 1: time maps compare by identity (G40), two builds of one timeline.
timeline = timeline_of([float(t) for t in range(8)], {0.0, 4.0})
doc = document(timeline)
print("1. two builds of one timeline are equal:", build_time_map(doc, timeline)[0] == build_time_map(doc, timeline)[0])
# Later: a start in the last 0.1 before the closing barline.
print("later: 4/4 ending at 12.0, positions(11.95, 12.0):", where(four_four.positions(11.95, 12.0)))
print("later: 4/4 ending at 12.0, positions(11.0, 11.95):", where(four_four.positions(11.0, 11.95)))
capped = time_map([float(t) for t in range(8)], {0.0, 4.0}, media_length=7.3)
print("later: last beat at 7.0, media 7.3, end:", capped.end)
for t in (7.15, 7.2, 7.25):
print(f"later: a point at {t}: {where(capped.positions(t, t))}")
# Later: a beat time that isn't a number.
timeline = beat_timeline([{"time": 0.0, "measure": {}}, {"time": "1.0"}, {"time": 2.0}])
try:
print("later:", build_time_map(document(timeline), timeline))
except Exception as error:
print("later: build_time_map raises:", type(error).__name__, error)
# Later: FrozenMetadata and a document's lists.
print("later: FrozenMetadata({'tags': ['a', 'b']}) == {'tags': ['a', 'b']}:", FrozenMetadata({"tags": ["a", "b"]}) == {"tags": ["a", "b"]})
print("later: FrozenMetadata({'tags': ['a', 'b']}) == {'tags': ('a', 'b')}:", FrozenMetadata({"tags": ["a", "b"]}) == {"tags": ("a", "b")})
# Later: a media length of 0.0.
records.clear()
time_map([float(t) for t in range(8)], {0.0, 4.0}, media_length=0.0)
print("later: media_length 0.0, warnings:", [r.getMessage() for r in records])Output:
1. beats at 4.0 (a downbeat) and 4.15, a point at 4.09: bar 2, beat 2, end_bar 2, downbeat True
1. beats at 4.0 (a downbeat) and 4.15, a point at 4.12: bar 2, beat 2, end_bar 2, downbeat False
1. time_map_for, a marker timeline with a measure table: None
1. build_time_map on the same timeline gives a map: True
1. 4/4, positions(4.0, 3.0): bar 2, beat 1, end_bar 2, downbeat True
1. bars 1, 2, 2c, 2, passes: [('1', 1), ('2', 1), ('2c', None), ('2', 2)]
1. two builds of one timeline are equal: False
later: 4/4 ending at 12.0, positions(11.95, 12.0): None
later: 4/4 ending at 12.0, positions(11.0, 11.95): bar 3, beat 4, end_bar 3, downbeat False
later: last beat at 7.0, media 7.3, end: 7.3
later: a point at 7.15: bar 2, beat 4.5, end_bar 2, downbeat False
later: a point at 7.2: None
later: a point at 7.25: None
later: build_time_map raises: TypeError unsupported operand type(s) for -: 'str' and 'float'
later: FrozenMetadata({'tags': ['a', 'b']}) == {'tags': ['a', 'b']}: False
later: FrozenMetadata({'tags': ['a', 'b']}) == {'tags': ('a', 'b')}: True
later: media_length 0.0, warnings: ['Timeline 01a124fa-60f1-7107-973e-8d7083d80251: the last beat (7.0) is at or after the end of the media (0.0); the last measure ends one gap after it.']
Every guarantee we checked: 101, of which 94 are caught and 7 untested
For each one, the code behind it was removed or neutralised, the core's 288 tests run on Python 3.12, and the code put back. Five of the table's counts differ from ours by one, probably because we removed the code differently: G07, G46, G89, G91 and G93. Each row is caught either way.
| Guarantee | Code | Tests failing | The PR's count | |
|---|---|---|---|---|
| G01 | No time map: no measure table (reason "no measure table") | timemap.py:159 |
1 | 1 |
| G02 | No time map: a downbeat without a time | timemap.py:162 |
1 | 1 |
| G03 | No time map: fewer than two beats | timemap.py:165 |
1 | 1 |
| G04 | No time map: two beats less than 1e-6 apart ("two beats at the same time") | timemap.py:168 |
1 | 1 |
| G05 | No time map: beats out of time order | timemap.py:170 |
1 | 1 |
| G06 | No time map: a folded table (rows[0].folded) | timemap.py:173 |
1 | 1 |
| G07 | A short unit (end snaps onto or before the start) ends in its start's bar | timemap.py:98 |
3 | 2 |
| G08 | Snapping uses tolerance plus a millionth (exactly 0.1 from a beat is on it) | timemap.py:121 |
1 | 1 |
| G09 | downbeat uses tolerance plus a millionth |
timemap.py:151 |
1 | 1 |
| G10 | A start off the map (before the first beat, or at/after the closing barline) has no Positions | timemap.py:133 |
3 | 3 |
| G11 | A start before the first beat has no Positions | timemap.py:133 |
1 | |
| G12 | A start at or after the closing barline has no Positions (the barline is excluded) | timemap.py:133 |
3 | |
| G13 | No end_bar for an end past the closing barline |
timemap.py:141 |
1 | 1 |
| G14 | An end exactly on the closing barline is in the last bar | timemap.py:141 |
3 | |
| G15 | end_bar excludes the end point: an end on a later downbeat belongs to the bar before |
timemap.py:143 |
3 | |
| G16 | The end is snapped before end_bar is found |
timemap.py:97 |
4 | |
| G17 | An end that is not after the start (even before it) ends in the start's bar, as a point's does | timemap.py:98 |
0, untested | |
| G18 | beat is fractional, linear between two beats |
timemap.py:105 |
6 | |
| G19 | beat is 1-based |
timemap.py:105 |
8 | |
| G20 | beat counts from the measure's first beat |
timemap.py:105 |
6 | |
| G21 | The fraction is relative to the beat's own length (linear) | timemap.py:136 |
2 | |
| G22 | A time on a beat is in that beat (bisect_right in _locate) |
timemap.py:135 |
8 | |
| G23 | downbeat tests the unsnapped start |
timemap.py:110 |
0, untested | |
| G24 | downbeat is true for a start just before a downbeat |
timemap.py:149 |
9 | |
| G25 | downbeat is true for a start just after a downbeat |
timemap.py:149 |
9 | |
| G26 | downbeat is false for a start that is far from every downbeat, and a start before the first downbeat is handled |
timemap.py:151 |
3 | |
| G27 | Snapping moves a time onto a beat up to 0.1 AFTER it | timemap.py:126 |
7 | |
| G28 | Snapping moves a time onto a beat up to 0.1 BEFORE it | timemap.py:124 |
8 | |
| G29 | Snapping moves a time onto the NEARER of two beats | timemap.py:126 |
1 | |
| G30 | The closing barline is a snap target (a time just before it is moved onto it) | timemap.py:120 |
2 | |
| G31 | A non-finite time (nan, inf) is no position (_snapped gives None) |
timemap.py:118 |
2 | |
| G32 | The tolerance is 0.1, in the file's unit | timemap.py:20 |
11 | |
| G33 | Positions.bar is the row's printed number | timemap.py:103 |
5 | |
| G34 | Positions.pass_ is the row's pass |
timemap.py:106 |
3 | |
| G35 | Positions.bar_count is the row's count |
timemap.py:107 |
3 | |
| G36 | Positions.bar_label is the row's label |
timemap.py:108 |
3 | |
| G37 | Positions.bar_beat_count is the row's beat count |
timemap.py:109 |
2 | |
| G38 | Positions.cadenza is the row's cadenza flag | timemap.py:111 |
1 | |
| G39 | Positions.movement is the row's movement | timemap.py:112 |
1 | |
| G40 | TimeMap compares by identity (eq=False) | timemap.py:45 |
0, untested | |
| G41 | TimeMap is immutable (frozen) | timemap.py:45 |
1 | |
| G42 | Positions is immutable (frozen) | timemap.py:29 |
0, untested | |
| G43 | MeasureRow is immutable (frozen) | table.py:46 |
1 | |
| G44 | TimeMap.start is the first beat | timemap.py:64 |
3 | |
| G45 | TimeMap.end is the closing barline | timemap.py:69 |
7 | |
| G46 | TimeMap.measure_rows() hands out a copy of the list |
timemap.py:78 |
1 | 2 |
| G47 | TimeMap.measures() gives (start, end, number, label) | timemap.py:82 |
2 | |
| G48 | The map's closing barline is the last row's end | timemap.py:179 |
1 | |
| G49 | The time map carries its timeline's id | timemap.py:176 |
1 | |
| G50 | The time map carries guessed as passed |
timemap.py:177 |
1 | |
| G51 | Each beat belongs to its measure (_measure_of) |
timemap.py:180 |
13 | |
| G52 | Each measure's first beat (_first) gives the beat number | timemap.py:181 |
6 | |
| G53 | Downbeats are the rows' starts | timemap.py:182 |
8 | |
| G54 | time_map_for: an unknown timeline id gives None |
__init__.py:15 |
1 | |
| G55 | time_map_for: a timeline that is not a beat timeline gives None |
__init__.py:15 |
0, untested | |
| G56 | time_map_for returns the map (not guessed) |
__init__.py:18 |
24 | |
| G57 | The package exports MeasureRow, Positions, TimeMap | __init__.py:6 |
0, untested | |
| G58 | count is the row's position in the timeline's table (kept when a measure is skipped) |
table.py:84 |
1 | 1 |
| G59 | A restart on the table's first row stays in movement 1 | table.py:93 |
1 | 1 |
| G60 | A restart starts every pass count again | table.py:94 |
4 | |
| G61 | A restart starts the next movement | table.py:95 |
4 | |
| G62 | A cadenza has no pass and doesn't count | table.py:97 |
2 | 2 |
| G63 | A cadenza has no pass (pass_ None), though it may count |
table.py:97 |
2 | |
| G64 | A cadenza doesn't count towards its number's passes | table.py:97 |
0, untested | |
| G65 | pass_ counts the times through a printed number, in timeline order |
table.py:98 |
7 | |
| G66 | pass_ counts per printed number (not over all measures) |
table.py:98 |
11 | |
| G67 | folded is set when any row has next |
table.py:85 |
3 | |
| G68 | folded is set on EVERY row when any row has next |
table.py:109 |
3 | |
| G69 | The last measure ends one gap after its last beat | table.py:129 |
19 | |
| G70 | The gap is taken from the last two beats of the table, even across measures | table.py:128 |
1 | |
| G71 | The closing barline is capped at the media's length when that comes sooner | table.py:142 |
2 | 2 |
| G72 | The media's length is used only in a seconds file | table.py:131 |
1 | 1 |
| G73 | A warning is logged when the media ends at or before the last beat | table.py:134 |
2 | 2 |
| G74 | The warning names the timeline's id (FR-082) | table.py:137 |
2 | |
| G75 | When the media ends at or before the last beat, the gap applies (the end is not capped) | table.py:136 |
2 | |
| G77 | A row ends at the next measure's downbeat | table.py:86 |
3 | |
| G78 | A row starts at its downbeat's time | table.py:110 |
9 | |
| G79 | A row's beats is the number of beats tapped |
table.py:113 |
15 | |
| G80 | A row carries its beat ids | table.py:112 |
1 | |
| G81 | A row carries restart |
table.py:108 |
2 | |
| G82 | A row carries cadenza |
table.py:107 |
3 | |
| G83 | A row carries source |
table.py:114 |
1 | |
| G84 | A row carries force_display |
table.py:115 |
1 | |
| G85 | A row carries next |
table.py:116 |
1 | |
| G86 | A row carries its label and number | table.py:104 |
10 | |
| G87 | build_rows of a timeline with no rows or no table is [] |
table.py:82 |
1 | |
| G88 | build_rows keeps the rows in the timeline's order |
table.py:75 |
25 | |
| G89 | Rows don't follow later edits: metadata is copied | table.py:25 |
3 | 4 |
| G90 | Row metadata's lists are copied as tuples (a list in the document can't be changed through a row) | table.py:26 |
2 | |
| G91 | Row metadata can't be changed | table.py:33 |
2 | 3 |
| G92 | Rows and time maps hash | table.py:39 |
1 | |
| G93 | Rows and time maps pickle and copy | table.py:28 |
2 | 3 |
| G94 | Metadata compares equal to a plain dict with the same items | table.py:33 |
7 | |
| G95 | find_rows: with neither a number nor a label it finds nothing |
table.py:155 |
1 | |
| G96 | find_rows: a number leaves cadenzas out |
table.py:162 |
1 | 1 |
| G97 | find_rows: a label finds its rows, cadenzas included |
table.py:162 |
2 | |
| G98 | find_rows: a number matches rows by number |
table.py:160 |
5 | |
| G99 | find_rows: a label matches rows by label |
table.py:161 |
4 | |
| G100 | find_rows: given both, both must match |
table.py:161 |
1 | |
| G101 | find_rows: pass_ narrows to one time through |
table.py:163 |
3 | |
| G102 | find_rows: a cadenza (no pass) is never found by a pass |
table.py:163 |
1 |
|
🤖 Claude One more commit, 0082bea: the reason "a downbeat without a time" now names the first such measure by its label, "a downbeat without a time (measure 2b)", so the user knows which beat to fix (TimeLineAnnotator/project-management#285). Tested in |
Adds tilia_core.timemap.table with MeasureRow, build_rows(document, timeline) and find_rows. Each row gets its position in the timeline, its pass (counted by printed number; none for a cadenza, and every count starts again at a restart, which also starts the next movement), whether the table is folded, and its start and end. The last measure ends one gap after its last beat, or at the end of the media if that comes sooner; no beats are added. find_rows finds rows by number (every pass, cadenzas left out), by label (cadenzas included) or both, narrowed by a pass. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ime map Adds TimeMap, Positions and time_map_for(document, timeline_id). positions(start, end) gives bar, end_bar, beat, pass, bar.count, bar.label, bar.beat_count, downbeat, cadenza and movement. A time within 0.1 (in the file's unit) of a beat or the closing barline is moved onto it; an end on a downbeat belongs to the bar before, a point's end_bar is its bar, and a start off the map has no position. A timeline with fewer than two beats, two beats at the same time, or a table with next gives no time map; build_time_map says why. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ons' tests Both test files built the same 4/4 piece with a one-beat pickup, in two different ways; docs.pickup_piece() now builds it once. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The media's length is in seconds, while a quarters file's times are in quarter notes, so comparing them would cut the last measure short or warn for nothing. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TimeMap.measure_rows() hands out the map's own rows, so a caller could change a shared time map through a row's metadata dict, and the dict made rows and time maps unhashable. The metadata is now a read-only mapping with its lists as tuples, left out of the hash, in place of a deep copy. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The helper made beats of kind "BEAT", the file's spelling, where the core's model names the kind "beat". Nothing read a beat's kind yet, so the tests passed, but a document they built couldn't have been written. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ess downbeat A beat timeline now gives no time map, with its reason, when: - two beats are less than 1e-6 apart: "two beats at the same time (1.0)"; - a beat comes before the one ahead of it: "beats out of time order (at 2.0)"; - it has no measure table, or beats but no rows: "no measure table"; - a measure's downbeat has no time: "a downbeat without a time". Before, beats out of order gave a map whose bisections were wrong, near duplicates split a measure, and the other two reported "fewer than two beats". A measure row's count is now its row's position in the table, so a skipped measure doesn't move the ones after it. Whether a table is folded is read from its rows, where it is computed. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A file that marks every song's first bar with restart, the first song's too, had no movement 1: the first restart moved the count on before any movement had begun. A restart on the table's first row now changes nothing. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a beat A unit was taken for a point only when its end equalled its start, and its end was snapped on its own. So an end that snapped onto the downbeat its start was on, or a point with float noise, gave end_bar the bar before (4.0 to 4.05: bar 2, end_bar 1), and a short unit on the first beat got no end_bar, as if it ran past the closing barline. Now an end that, once both ends are snapped, isn't after the start gives end_bar the start's bar. A time exactly the tolerance from a beat was snapped or not depending on float rounding (3.9 stayed in bar 1, 7.9 went onto bar 3's downbeat). The tolerance is now 0.1 plus a millionth, for snapping and for downbeat. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A row's metadata was a mappingproxy, so pickle.dumps, copy.deepcopy and dataclasses.asdict of a row or a time map raised TypeError. It is now FrozenMetadata, a read-only mapping with its lists as tuples, which hashes, pickles and copies. TimeMap's start, end and tolerance are now properties read from its beat slots and TOLERANCE, so dataclasses.replace can't make them disagree, and TimeMap no longer generates an __eq__ and __hash__ that walk every row and beat. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason now says which measure: "a downbeat without a time (measure 2b)", by the first such measure's label, so that the user, or TiLiA asking the user, knows which beat to fix. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A beat that wasn't a downbeat and had no time was dropped with a log line, and the map was built anyway: the beats after it in its bar were renumbered. A NaN time passed every check, and a time that wasn't a number made build_time_map raise. Now every beat of the table is checked first, and a missing, NaN or non-number time gives "a beat without a time (measure 1)", or "a downbeat without a time (measure 2)" on a downbeat, naming the measure by its label. A row whose beats don't start at its downbeat, which only a broken table has, gives "the measure table doesn't match its beats (measure 2)" instead of the downbeat's reason. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A row's metadata was a Mapping of its own: dataclasses.asdict kept it as is, so json.dumps of a row failed, and pickle protocols 0 and 1 refused it. It is now a dict subclass whose changing methods raise TypeError, with __reduce__ for every pickle protocol, so asdict and json.dumps work. Its lists stay tuples, so it still hashes, and it compares unequal to a dict holding the same values as lists. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
build_time_map built every row of a folded table only to discard it, and could log the media-length warning for a map it never built. Both it and build_rows now ask is_folded(rows), which keeps the rule in one place, and build_time_map asks before building anything. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TimeMap took its beat slots, each beat's measure, each measure's first beat and the downbeats as constructor fields, so dataclasses.replace could make them disagree with its rows. It now takes only its rows and beat times, and computes the rest in __post_init__ (init=False fields, which replace refuses), checking that the rows' beats and the times match. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… downbeat positions() only asks _end_bar about an end after the start, but without the guard any other caller would get the last bar for an end at or before the first downbeat (bisect - 1 is -1). It gives None again. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dataclasses.replace refuses an init=False field with ValueError before Python 3.13 and with TypeError from 3.13 on, so the test of the time map's computed arrays failed on 3.13 and 3.14. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8550093 to
46ec198
Compare
|
🤖 Claude The second code review, on the five fix commits, found more. These are fixed, each with its test written first. The branch is also rebased onto the current
Left as they are: 304 tests pass on Python 3.10, 3.11 and 3.13, and Commit ids before and after the rebase
|
build_rows still built rows for a timeline that gets no time map because a beat has no usable time: a measure lost the beat, a measure whose downbeat had none was dropped and the one before stretched over it, and a NaN time could reach a row's start or end. Imports, plans and stored tables would act on those rows. build_rows now gives none, and missing_time(timeline), which build_time_map uses for its reason, says why. A row skipped because its beats don't start at its downbeat, which only a broken table has, still leaves the others their count. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A song that starts with a pickup starts at the pickup, so the pickup carries the restart: two such songs give movements 1, 1, 1, 2, 2, 2, and every pass 1. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude Two more commits, both decided by Anne (2026-10-12):
311 tests pass on Python 3.10, 3.11 and 3.13, and |
A row whose beats don't start at its downbeat, or that has no beats, was left out of the rows while the others were kept. Only a broken table has one, and the time map already refused it. broken_row(timeline) now finds it, and build_rows gives no rows and build_time_map its reason, as for a beat without a usable time. Since rows_of then never leaves a row out, a row's count is its index + 1 again, and the lookup by downbeat id goes. Part of TimeLineAnnotator/project-management#211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Claude One more, decided by Anne (2026-10-12): no measure rows for a broken table either (98b4253). A row whose beats don't start at its downbeat, or that has no beats, was left out while the others were kept. |
🤖 Claude
Computes a beat timeline's measure table and the time map built on it, in
tilia_core.timemap.What changed
timemap/table.py(new)MeasureRow, a frozen dataclass:id,count,number,label,pass_,movement,cadenza,restart,folded,start,end,beat_ids,beats,source,force_display,next,metadata.build_rows(document, timeline):missing_time(timeline)andbroken_row(timeline)name the measure, andbuild_time_mapgives the same reason.countis the row's position in the timeline's measure table.pass_counts the times through a printed number, in timeline order. A cadenza has no pass and doesn't count. A restart starts every count again and starts the next movement, except on the table's first row, which stays in movement 1.foldedis set on every row when any row hasnext(is_folded).FrozenMetadata: each row's metadata as a read-only copy. It is adictwhose changing methods raiseTypeError, with its lists as tuples. A built table doesn't follow later edits to the document, nobody can change a shared time map through it, and rows hash, pickle with any protocol, copy, and turn into JSON throughdataclasses.asdict.find_rows(rows, number=None, *, label=None, pass_=None):pass_narrows to one time through.timemap/timemap.py(new)TimeMap, a frozen dataclass built from its rows and beat times, withtimeline_idandguessed, and the propertiesstart(the first beat),end(the closing barline) andtolerance(0.1, in the file's unit).measure_rows(),measures()((start, end, number, label)) andpositions(start, end).__post_init__, and every lookup is a bisection.Positions, a frozen dataclass:bar,end_bar,beat,pass_,bar_count,bar_label,bar_beat_count,downbeat,cadenza,movement.beatis 1-based and fractional, linear between two beats.end_bar. The end point is excluded, so an end on a later downbeat belongs to the bar before. An end that, once both ends are snapped, isn't after the start ends in the start's bar, as a point's does. An end past the closing barline has noend_bar.downbeattests the unsnapped start against the downbeats, with the same tolerance.Positions.build_time_map(document, timeline, *, guessed=False)returns the time map, orNoneand the reason there is none:next), which positions don't handle yet", given before any row is built.timemap/__init__.py:time_map_for(document, timeline_id), and exportsMeasureRow,PositionsandTimeMap.tests/timemap/docs.py: beats get the core's kind name,"beat", andpickup_piece()builds a piece both test files use.Tests
tests/timemap/test_timemap_table.pyandtests/timemap/test_timemap_positions.py. They cover:find_rows;end_bar, short units and units on the first beat;uv run --package tilia-core pytest packages/tilia-core -q: 312 passed on Python 3.11, and on 3.10 and 3.13 withuv run --isolated --python <version>.python scripts/check_packages.py, run with Python 3.14.6: all package checks passed.Each guarantee, with its code neutralised. For each one, the code that provides it was replaced (the condition by
False, or by the code before the fix), the package's tests were run, and the code was restored. Rows up tomeasure_rows()were run with 291 tests, before the rebase onto the currentv1-prerelease; the later ones with 311 or 312:downbeatend_barpast the closing barlineend_barat or before the first downbeat (_end_bar's guard)countis the row's position in the tablefind_rows: a number leaves cadenzas outFrozenMetadata: item assignment refusedFrozenMetadata: changing methods refusedFrozenMetadata: pickles with any protocolmeasure_rows()hands out a copy of the listPart of TimeLineAnnotator/project-management#211
🤖 Generated with Claude Code