Conversation
nextPitch spelled the found node's degree from the scale's own tonic and
moved it into the origin's octave. That holds when the network repeats
at the octave, but in a cyclical one a degree stands for more than one
pitch class: every note of a cycle of major thirds is the same node. So
the walk began on the wrong pitch.
>>> sc = scale.CyclicalScale('d3', ['M3'])
>>> sc.nextPitch('g4')
<music21.pitch.Pitch F#4>
>>> sc.nextPitch('c4')
<music21.pitch.Pitch F#3>
For a network that is not octave-duplicating, take the node's pitch from
a realization around the origin instead. Octave-duplicating networks are
unchanged.
|
coverage: 93.368% (+0.002%) from 93.366% — float3:next-pitch-on-cycles into cuthbertLab:master |
|
Is that example a before or after? If after it doesn't seem right. |
|
that's before. |
|
Oh good! |
mscuthbert
left a comment
mscuthbert
left a comment
There was a problem hiding this comment.
Some requests for clarification etc. thanks!
| Read off a realization around the origin, since in a network that does | ||
| not repeat at the octave one node can stand for several pitch classes: | ||
| every note of a cycle of major thirds is the same node. |
There was a problem hiding this comment.
This comment doesn't seem right "every note of a cycle of major thirds is the same node" -- say we start on D3 shouldn't all Ds be the same node, and all F#s be the same, but not that all Ds are the same as all F#s?
| # realize the pitch from the found node degree | ||
| # we may be getting an altered | ||
| # tone, and we need to transpose an unaltered tone, thus | ||
| # leave out altered nodes argument | ||
| p = self.getPitchFromNodeDegree( | ||
| pitchReference=pitchReference, | ||
| nodeName=nodeName, |
There was a problem hiding this comment.
In the future, it would be a big help to my review if you put GH comments like "moved into the else: below" so I'm not wondering why all this is gone. Thanks!
| # we may be getting an altered | ||
| # tone, and we need to transpose an unaltered tone, thus |
There was a problem hiding this comment.
again -- this is a great argument for removing alteredDegrees altogether. --we might be able to use _nodePitchNearOrigin for all.
| or (not usedNeighbor and direction == Direction.ASCENDING)) | ||
| p: pitch.Pitch|None | ||
| if not self.octaveDuplicating: | ||
| p = self._nodePitchNearOrigin(pitchReference, |
There was a problem hiding this comment.
preexisting error, but seems odd that CyclicalScale(..., ['M3']) doesn't create an octaveDuplicating network. 'P4' I get...
There was a problem hiding this comment.
If this is true, but a note here that this path also works for octaveDuplicating scales but it is slower. I'm wondering why we don't simplify to one pathway and then optimize the hell out of that instead? :-)
|
|
||
| fifths = scale.CyclicalScale('c4', ['P5']) | ||
| self.assertEqual(str(fifths.nextPitch('g4')), 'D5') | ||
| self.assertEqual(str(fifths.nextPitch('g4', Direction.DESCENDING)), 'C4') |
There was a problem hiding this comment.
Thanks for doing this test too -- important to see non-octave repeated here.
Can we also add a test on multiple intervals that don't add up to an octave, like scale.CyclicalScale('d3', ['M3', 'm3']) so we can be sure that we're respecting the different intervals.
And also nextPitch tests on enharmonics (both higher DNN and lower DNN) still go to the correct pitches. thirds nextPitch on C##4 goes to F#4 not D4, etc.
Thanks!
nextPitch now takes the node's pitch from a realization around the origin
for octave-duplicating networks too, which removes the second path. It
also corrects enharmonics across an octave boundary there:
MajorScale('c4').nextPitch('b#3') was D3 and is D4.
Tests for a cycle of two intervals and for enharmonic origins, and a
clearer docstring for _nodePitchNearOrigin.
realizeAscending cached a realization under its exact range, so every new origin given to nextPitch walked its two octaves again. The walk depends only on its node, its starting pitch and the altered degrees, so it is now kept under those and extended as far as a range needs, and each range is filtered from it. Networks pickled before the walks existed make theirs on first use. nextPitch on a fresh scale over 36 origins, against master: MajorScale 1045 -> 393 us, HarmonicMinorScale 1141 -> 419 us, CyclicalScale M3 558 -> 348 us. A single getPitches is unchanged.
3 participants
nextPitchon a cyclical scale could start below the origin:Non-octave-duplicating networks now find the node's pitch near the origin (D4 here). About 15% slower for cyclical scales; others unchanged.
AI-assisted (Claude).