Hand back a new pitch from getPitchFromNodeDegree - #2028
Conversation
realizeAscending() and realizeDescending() return the very list they put
in _ascendingCache / _descendingCache, so the pitches inside it belong to
the cache. getPitchFromNodeDegree() handed one of those pitches straight
back, and nextPitch() writes the origin's octave onto the pitch it gets
("transfer octave from origin to new pitch derived from node"), so it was
editing the cached realization in place.
The effect is that a scale answers differently depending on what has been
asked of it before:
>>> sc = scale.RagAsawari('c4')
>>> str(sc.pitchFromDegree(1))
'C4'
>>> str(sc.nextPitch('c1', scale.Direction.ASCENDING))
'D1'
>>> str(sc.pitchFromDegree(1))
'C1'
The cached entry for ('C4', 'C4', 'C5') has its first pitch moved from C4
to C1 by the walk near C1, and every later reader of that realization
sees it. realize() already guards the list against this ("Make new
objects, because this value might be cached ... and mutating it would be
dangerous"); this is the same hazard one level down, on the pitches.
test_scale_main.testRagAsawari asserted the corrupted 'C1'. It asserts
'C4' now, which is what the same call returns earlier in that very test.
Two regression tests go with it: one that the pitch handed back is the
caller's own, one for the reported symptom.
The whole suite is unchanged otherwise: 4,597 tests, one failure fewer
and nothing newly failing.
|
Hi -- the reuse of cached objects here is well -known. Please ensure that the timing considerations from making these changes are considered and demonstrated in the proposal. I specifically have not implemented most changes like this so far because they slow down the generation of scales and work on scales so much that the safety isn't worth the practical considerations. Please write what tests you performed on timing and how the system reacted before and after. Primarily with the creation of MajorScale and various minor scales, since those are the two scales created en masse. |
|
coverage: 93.349% (+0.001%) from 93.348% — float3:fix-interval-network-cache-aliasing into cuthbertLab:master |
|
Timings below. Short version: the four scales you named never reach the changed Method:
The three bold rows are the load-bearing ones — they reach the changed function on Several deltas are negative, so here is the noise floor — the same baseline build
Run-to-run spread on identical code is ±1.3%, which is the same size as every Why it is invisible where it does apply: If the 2.70 µs is still unwanted on the |
|
Hi Float3 -- I know the answer, and you know the answer, but since it's very "load bearing" on your contributions. Please read the Contributing guide and declare use of AI or not and then reopen. I'm appreciating the quality, but you need to own your contributions and edit them so a human can read them. It's a volunteer project and just the rules here. (cool website btw). |
|
hi, yeah, I didn't read the contributing.md, I've now read it. Sorry about that. To retroactively declare AI-use I have used AI in some part or another of the process of all my recent PRs here. I told Claude to post the timing table in the comment and it posted more than I wanted. Sorry about that. I really like music21 and hope I can continue to contribute in the future, now following the rules. (I'm glad you like the website) |
No worries! I'm trying to be a stickler on that because of some other really non-productive requests and I'd been looking aside for a bit but the "load bearing" reply was just too obvious to ignore. In generally though if you could ask your Claude to be more concise and useful for overburdened human code maintainers, that'd be helpful. Glad to know that this can now be done w/o major timing problems. Will review soon and either get back w/ suggestions or merge. |
mscuthbert
left a comment
mscuthbert
left a comment
There was a problem hiding this comment.
Great!
two followups here:
AbstractScale.getPitchFromNodeDegree, AbstractScale.getNewTonicPitch -- make deepcopies that are no longer needed -- there is one place with an actual 12% slowdown : .pitchFromDegree with minPitch and maxPitch specified, but that's not that common, and only 1 microsec. so fine.
doc stryle: adversarial review did catch "The three-line comment added in test_scale_main.py and the docstring of the second new test narrate the old bug. House style keeps that history in the commit message, which already tells it well." -- so it is something that your agent should have picked up on -- same with intervalNetwork 2710-2713 -- narrating old bug.
| getNeighbor=Direction.DESCENDING)), | ||
| 'F1') | ||
|
|
||
| self.assertEqual(str(sc.pitchFromDegree(1)), 'C1') |
There was a problem hiding this comment.
from .agents/skills/writing-docs/SKILL.md
Say what is, not what was or what not to do
Describe current behavior. Do not narrate the bug you just fixed, the old
spelling of an API, or when upstream changed something.
more there for the agent to digest... :-)
getPitchFromNodeDegree hands back a new pitch, so the AbstractScale wrappers over it, getPitchFromNodeDegree and getNewTonicPitch, no longer need a copy of their own. The comments in intervalNetwork, test_intervalNetwork and testRagAsawari told the story of the bug that copy fixed; the fix's own commit already tells it, so they go.
3 participants
realizeAscending() and realizeDescending() return the very list they put in _ascendingCache / _descendingCache, so the pitches inside it belong to the cache. getPitchFromNodeDegree() handed one of those pitches straight back, and nextPitch() writes the origin's octave onto the pitch it gets ("transfer octave from origin to new pitch derived from node"), so it was editing the cached realization in place.
The effect is that a scale answers differently depending on what has been asked of it before:
The cached entry for ('C4', 'C4', 'C5') has its first pitch moved from C4 to C1 by the walk near C1, and every later reader of that realization sees it. realize() already guards the list against this ("Make new objects, because this value might be cached ... and mutating it would be dangerous"); this is the same hazard one level down, on the pitches.
test_scale_main.testRagAsawari asserted the corrupted 'C1'. It asserts 'C4' now, which is what the same call returns earlier in that very test.