refactor: beat measure-recomputation suppression, and the staleness it was hiding - #602
Open
FelipeDefensor wants to merge 6 commits into
Open
refactor: beat measure-recomputation suppression, and the staleness it was hiding#602FelipeDefensor wants to merge 6 commits into
FelipeDefensor wants to merge 6 commits into
Conversation
_figs_to_str emitted an "s" for every figured-bass slot without an accidental. MusAnalysis consumes that placeholder only inside the "%" stack, and only in its exact shape: three slots of one character each, paired positionally with three numbers. Anywhere else it is drawn as a literal letter -- "Is6" for a first-inversion triad, "Vss42" for a third-inversion dominant seventh. Take the stack only for that exact shape and fall back to the inline form otherwise. The length test alone missed two further cases that also overflow it: groups of four or more figures, and three-figure groups where a double accidental needs two characters for one slot. Over every quality x step x root accidental x inversion in C major and A minor (10500 labels): malformed "%" stacks 1728 -> 0, stray "s" outside a stack 988 -> 0. Every remaining "s" is either a consumed blank slot in a well-formed stack or part of the Tristan chord's spelled-out name. The accidentals of a non-stacked group are still emitted as a block before its numbers, so an accidental can be drawn against the wrong figure. That is pre-existing and tracked separately in #598.
BeatTimeline.delete_components disabled the component manager's compute_is_first_in_measure flag for the duration of the deletion, but re-enabled a misspelled attribute (update_is_first_in_measure). The real flag stayed False for the rest of the timeline's life, so every beat created afterwards skipped recalculate_measures() and kept the default is_first_in_measure = False: no measure numbers, no long dashes. Deleting *all* beats made it worse. The recompute was guarded by `if not self.is_empty:`, so an emptied timeline kept a stale beats_in_measure / measure_numbers describing beats that no longer existed. Suppress the flag through a context manager that restores the value it found rather than a literal True, and yields that value so the caller can tell whether it owns the recomputation. This matters because BeatTLComponentManager.restore_state suppresses recomputation across a whole delete-then-create pass and reaches delete_components in the middle of it: restoring True there re-enables the per-beat recomputation for every beat the restore re-creates afterwards. Restoring 100 beats went from 1 recalculate_measures() call to 101, and from 0.010s to 0.078s, growing quadratically. The finally clause also keeps a raising delete or listener from leaving the flag off for the rest of the session, which would silently reinstate the original symptom. Move recalculate_measures() out of the emptiness guard so an emptied timeline drops its stale measure data and the UI clears its labels. The guard is then dead: update_is_first_in_measure_of_subsequent_beats already iterates nothing on an empty timeline, and both sibling call sites invoke it unguarded. crop() and clear() suppress the same flag without ever recomputing, so they still leave a measure structure describing beats that no longer exist. Left alone here to keep the hotfix narrow.
TestPageNumber.test_first_marker_page_number_is_one and test_marker_page_number_default_is_next_page called commands.execute() but only requested the pdf_tl fixture, not pdf_tlui. Without pdf_tlui (which depends on tluis -> qtui), nothing instantiates TimelineUIs, so "timeline.pdf.add" is never registered and the call raises/silently no-ops depending on ordering. Every sibling test in the class already requests pdf_tlui for this reason; these two were the only holdouts, which only showed up as a failure once test ordering (or PR #538's xdist scheduling shift) put them first in the module.
BeatTLComponentManager carries two flags that callers switch off around a bulk operation and back on afterwards. Every one of those windows was a pair of bare attribute assignments that restored a literal True rather than the value found on entry, with no try/finally. That shape is what produced the bug fixed in the previous commit: a misspelled restore (update_is_first_in_measure) silently created a new attribute and left the real flag off for the rest of the timeline's life, and nothing in CI could see it -- the project runs black and ruff with E,W,F,B,I selected, none of which flag an assignment to an attribute that does not exist, and there is no type checker configured. Route all seven windows through suppressing_is_first_in_measure() and the new suppressing_metric_fraction_dict(). Both restore what they found and do so in a finally clause, so a raising body can no longer wedge a flag off. This is behaviour-preserving. Instrumenting every window over a full test run, only delete_components is ever entered with a flag already off (17 of its 32 calls, from restore_state); crop, clear, deserialize_components, restore_state, fill_with_beats and update_is_first_in_measure_of_subsequent_beats find True on entry every single time, so restoring the entry value instead of True cannot change what they do.
Both suppress compute_is_first_in_measure around a bulk deletion and then never recompute, so the timeline keeps a beats_in_measure and measure_numbers describing beats that no longer exist. crop() runs whenever the loaded media gets shorter and the user declines scaling. Twelve beats at pattern 4 cropped to 7.5s leaves 8 beats but still reports measure_count 3, beats_that_start_measures [0, 4, 8], and a metric-fraction dict holding the deleted beats. get_time_by_measure(3) then returns a time with no beat, which misplaces every by-measure CSV and MusicXML import, and "Distribute beats" on the last visible measure raises IndexError because the last-measure guard is fooled by the phantom third measure. Saving in that state serializes the phantom too. clear() is reached from CSV/MusicXML import into an existing beat timeline, from the "delete existing beats?" confirmation in Fill with beats, and from Timelines.clear_timelines. After it the timeline has no beats but still reports three measures, so the import or fill that follows adjusts a stale list instead of building from empty. Extract the recalculate/re-derive/notify trio the fixed delete path already performs into BeatTimeline.refresh_measures() and call it from crop, clear, delete_components and restore_state. Those four had four hand-written variants of the same sequence, differing in whether they posted and whether they guarded on emptiness; that divergence is how delete_components came to be missing its recomputation in the first place. fill_with_beats and deserialize_components are left alone: their sequences genuinely differ (no post, and a different post respectively).
BeatTimelineUI.on_delete_component called recalculate_measures() after super().on_delete_component, which routes unconditionally through BeatTimeline.delete_components -- and that now performs the recalculation itself. The override was a pure pass-through plus a duplicate. The duplicate is not cheap. recalculate_measures starts by clearing every cached metric position, so the second pass throws away the caches the first one just built and re-derives all of them; each cold Beat.metric_position resolves itself with components.index(beat), which copies the component list and scans it. That is O(n^2) per pass, paid twice on the most common delete path. Drop the override and inherit the base implementation.
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
2 participants
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.
Follow-up to the v0.6.5 hotfix. Three commits, each with its own rationale.
1.
refactor: route every suppression window through a context managerBeatTLComponentManagercarries two flags that callers switch off around a bulk operation and back on afterwards. Every one of those windows was a pair of bare attribute assignments restoring a literalTrue, with notry/finally.That shape is what produced the bug this hotfix fixed: a misspelled restore (
update_is_first_in_measure) silently created a new attribute and left the real flag off for the rest of the timeline's life. Nothing in CI could see it — the project runs black and ruff withE,W,F,B,I, none of which flag an assignment to an attribute that does not exist, and there is no type checker configured.This commit is behaviour-preserving, and that is measured rather than asserted. Instrumenting every window over a full test run:
Trueclearcropdeserialize_componentsrestore_statefill_with_beatsupdate_is_first_in_measure_of_subsequent_beatsdelete_componentsOnly
delete_componentsever nests, and that is the one the hotfix already addressed. For the other six, restoring the entry value instead ofTruecannot change what they do.2.
fix:crop()andclear()leave a stale measure structure behindBoth suppress the flag around a bulk deletion and then never recompute.
crop()runs whenever the loaded media gets shorter and the user declines scaling. Twelve beats at pattern 4 cropped to 7.5s leaves 8 beats but still reportsmeasure_count3 andbeats_that_start_measures[0, 4, 8].get_time_by_measure(3)then returns a time with no beat, which misplaces by-measure CSV and MusicXML imports, and "Distribute beats" on the last visible measure raisesIndexErrorbecause the last-measure guard is fooled by the phantom third measure. Saving in that state serializes the phantom.clear()is reached from import into an existing beat timeline, from the "delete existing beats?" confirmation in Fill with beats, and fromTimelines.clear_timelines. After it the timeline has no beats but still reports three measures.Both defects predate the hotfix. The commit also extracts
BeatTimeline.refresh_measures()from the four sites that had hand-written variants of the same recalculate/re-derive/notify trio — that divergence is howdelete_componentscame to be missing its recomputation in the first place.fill_with_beatsanddeserialize_componentsare left alone; their sequences genuinely differ.3.
perf: stop recalculating measures twice on every deletionBeatTimelineUI.on_delete_componentcalledrecalculate_measures()aftersuper().on_delete_component, which routes unconditionally throughBeatTimeline.delete_components— and that now performs the recalculation itself.recalculate_measuresstarts by clearing every cached metric position, so the second pass throws away the caches the first just built and re-derives all of them, each via an O(n)components.index(). The override is dropped.Testing
Full suite green (1370 passed). Both fixes in commit 2 have a counter-check: reverting the
refresh_measures()calls makes exactly the two new tests fail, and nothing else.