(AI) Preserve microtones through Interval.transpose (fix accidental corruption / AccidentalException)#1962
Conversation
Pitch.transpose() corrupted, or crashed on, pitches carrying a microtone.
_diatonicTransposePitch measured interval2 from the microtonal source
pitch, so the source cents leaked into halfStepsToFix and were dumped onto
the accidental: e.g. C4(+30c) transposed by a perfect unison became
C~4(-20c), and G#5(+50c) by an augmented unison raised AccidentalException
('2.5 is not a supported accidental type'). The dead 'centsOrigin' comment
in the method showed this was an unfinished intent.
Peel the source microtone off halfStepsToFix before it sets the accidental,
then restore it as a microtone, guarded by 'if centsOrigin' so the common
non-microtonal (performance-critical) path is unchanged.
mscuthbert
left a comment
There was a problem hiding this comment.
Great PR -- a few smaller changes requested, with guidance to you and agent for future PRs.
Btw -- version 11 or a future version might deprecate the ~ and ` representations of half-flats and sharps, so if you want to make future microtonal contributions please use Accidental('half-flat') instead of ``` etc. Thanks!
| pitch2 = copy.deepcopy(pitch1) | ||
| oldDiatonicNum = pitch1.diatonicNoteNum | ||
| # centsOrigin = pitch1.microtone.cents # unused!! | ||
| centsOrigin = pitch1.microtone.cents |
There was a problem hiding this comment.
this should gate on if not pitch1.isTwelveTone(): ... else: centsOrigin = 0.0
There was a problem hiding this comment.
I have a patch that should make isTwelveTone run faster -- will push in next PR. isTwelveTone guards against creating Microtone objects unnecessarily.
| # We have the right note name but not the right accidental | ||
| interval2 = Interval(pitch1, pitch2) | ||
| # halfStepsToFix already has any microtones | ||
| # interval2 is measured from the microtonal pitch1, so halfStepsToFix |
There was a problem hiding this comment.
pitch1 isn't necessarily microtonal. This comments should be trimmed to essentials and moved into the if centsOrigin: clause -- in general agents write too much about their particular task and leave too much baggage for people interested in other tasks. Think about 10-15 words
| and oldPitch2Accidental.name == 'natural'): | ||
| pitch2.accidental = oldPitch2Accidental | ||
|
|
||
| # restore the source microtone that was peeled off halfStepsToFix above |
There was a problem hiding this comment.
this does not restore the source microtone, it restores the microtone's .cents. And again comment inside the if.
|
|
||
| # restore the source microtone that was peeled off halfStepsToFix above | ||
| if centsOrigin: | ||
| pitch2.microtone = pitch2.microtone.cents + centsOrigin |
There was a problem hiding this comment.
pitch2.microtone.cents = ...
| i = interval.Interval(note.Note('c4'), note.Note('c~4')) | ||
| self.assertEqual(str(i), '<music21.interval.Interval A1 (-50c)>') | ||
|
|
||
| def testTransposeMicrotonePreserved(self): |
The bug
Transposing a
Pitch(orNote) that carries a microtone corrupts its spelling, and for larger microtones raises an exception — even when the transposition is a no-op:Transposing by a perfect unison must be the identity, so
C4(+30c)→P1→C4(+30c)is unambiguous. The corruption kicks in above 25¢ (25¢ survives, 26¢ flips to a half-sharp) and affects ordinary intervals too —Note('C4', microtone=30).transpose('P5')givesG~4(-20c)instead ofG4(+30c). It's also internally inconsistent: a 25¢ microtone is preserved byP5, and a 30¢ microtone is preserved by intervals that resolve to a sharp/flat — so identical inputs behave differently depending on the interval.Root cause
music21/interval.py,Interval._diatonicTransposePitch:interval2is measured from the microtonalpitch1, so the source cents (0.30 semitone) leak intohalfStepsToFix. That fractional value is then assigned to the accidental, which snaps to the nearest quarter-tone (half-sharp = 0.5) and dumps the −0.20 remainder into a spurious microtone — or, when the alter lands on an unsupported value like 2.5, raisesAccidentalException.The method even contains a dead comment,
# centsOrigin = pitch1.microtone.cents # unused!!, marking the intended-but-never-wired-up fix.The fix
Resurrect
centsOrigin, peel it offhalfStepsToFixbefore the accidental is set, and restore it as a microtone afterward. The whole thing is guarded byif centsOrigin:so the common, non-microtonal (performance-critical) path is byte-identical:Now
C4(+30c)→P1→C4(+30c),G#5(+50c)→A1→G##5(+50c)(no crash), and a combined quarter-tone-plus-microtone pitch likeC~4(+30c)round-trips through a unison unchanged.Tests
testTransposeMicrotonePreservedcovers the unison identity, the formerly-crashingA1case, a combined accidental+microtone pitch, and a sweep over eight intervals × four microtone magnitudes asserting the cents are preserved, the sounding pitch (ps) moves by exactly the interval, and no quarter-tone accidental is introduced on an integer-semitone interval. It fails onmasterand passes with the fix. The existinginterval,pitch, andnotetest/doctest suites stay green;ruff,pylint -j0, andmypyare clean.Disclosure (per CONTRIBUTING.md): prepared with AI assistance under my direction. I reproduced the bug, wrote and reviewed the fix and tests, and verified it locally — full
interval/pitch/notesuites, ruff/pylint/mypy, and an ~1,200-case oracle (intervals × microtones × accidentals) with zero microtone/ps failures and zero crashes.