Romantext treatment of transposed roman numerals in copied measures

[malcolmsailor]

music21 version

8.0.0a12

Problem summary

When measures are copied when parsing romantext files using the "m3=m2" syntax, roman numerals are created that share keys; this leads to funky behavior when transposing the file.

Steps to reproduce

>>> import music21
>>> key = "a"
>>> rntxt = f"""Time Signature: 2/4
... m1 {key}: i
... m2 V42/V
... m3 = m2
... """
>>> score = music21.converter.parse(rntxt, format="romanText")
>>> transposed = score.transpose(2)
>>> transposed.show("text")
{0.0} <music21.metadata.Metadata object at 0x102edcf40>
{0.0} <music21.stream.Part 0x102edca30>
    {0.0} <music21.stream.Measure 1 offset=0.0>
        {0.0} <music21.key.Key of b minor>
        {0.0} <music21.meter.TimeSignature 2/4>
        {0.0} <music21.roman.RomanNumeral i in a minor>
    {2.0} <music21.stream.Measure 2 offset=2.0>
        {0.0} <music21.roman.RomanNumeral V42/V in a minor>
    {4.0} <music21.stream.Measure 3 offset=4.0>
        {0.0} <music21.roman.RomanNumeral V42/V in b minor>

Note "b minor" for the last roman numeral but a minor for all the others.

This might seem to be a good thing---at least this roman numeral is transposing, while the others are not. But when we implement roman numeral transposition, it will lead to such roman numerals being transposed twice, because first the key is transposed, and then the roman numeral will be transposed. E.g., we get

{0.0} <music21.metadata.Metadata object at 0x100dd0f40>
{0.0} <music21.stream.Part 0x100dd0a30>
    {0.0} <music21.stream.Measure 1 offset=0.0>
        {0.0} <music21.key.Key of b minor>
        {0.0} <music21.meter.TimeSignature 2/4>
        {0.0} <music21.roman.RomanNumeral i in b minor>
    {2.0} <music21.stream.Measure 2 offset=2.0>
        {0.0} <music21.roman.RomanNumeral V42/V in b minor>
    {4.0} <music21.stream.Measure 3 offset=4.0>
        {0.0} <music21.roman.RomanNumeral V42/V in c# minor>

The solution appears to be to make a copy of 'kCurrent' whenever we create a new RN in _copySingleMeasure and _copyMultipleMeasures in romanText/translate.py. I updated my PR #1414 to address this, as well as updated Roman Numeral transposition based on the discussion in #1413.

[mscuthbert]

Just when working through this problem know that Key creation is one of the slowest parts of music21, so any solution that involves "Create a new Key object for every RN" would need to be preceded by a PR of "Speed up Key creation 10x" -- actually the slowness of Key creation is not in KeySignature creation but Scale creation. There are some opportunities for speeding up Scale creation (special-casing major and minor scales is probably the optimal part of it, or sharing one IntervalNetwork object) but those would need to happen first.

[malcolmsailor]

I hear what you're saying, but surely it's better for it to work slowly than for it to not work at all? Or if you don't want to implement Roman Numeral transposition, we could at least have it raise a NotImplementedError to save unsuspecting users such as myself the hassle of debugging why things are not working as expected?

Moreover, I'm a bit confused because in #1413 when I suggested making a new key for every transposed RN you endorsed the idea.

Also, roman text parsing already creates a new key object for every RomanNumeral it creates (e.g., here). The only change my PR makes to romanText/translate.py is to make this behavior uniform in the unusual case when a roman numeral:

1. has a secondary roman numeral
2. is part of a "m2=m1" type copy operation

In the current code, this seems to be the only situation where the romantext parser doesn't already create a new roman numeral.

It seems like it might be possible to change the romantext parser so that romannumerals share keys. Then transposition could just transpose the key objects, and not the roman numerals themselves. But that would be a bigger change and I don't know what else it would potentially break. For my own work, I need transposition of roman numerals and romantext-parsed scores to work correctly and I thought that would potentially be useful to others as well, hence I made this quick PR.

[mscuthbert]

surely it's better for it to work slowly than for it to not work at all?

Currently RomanNumerals work properly for all use cases except transposition -- I take issue with saying that they don't work at all. Your proposal will slow down all other cases except yours.

[malcolmsailor]

Hi Myke, I certainly didn't mean to suggest that roman numerals don't work in general and I'm sorry if it came off that way. That's why I suggested in the next sentence that roman numeral transposition raise a NotImplementedError, at least temporarily, if you prefer not to accept my PR.

My PR creates new key objects in precisely two cases (in fact, in the latter case, to be precise, an existing key is copied with deepcopy, which may be faster than creating a new key from scratch):

1. when transposing a roman numeral. This will not slow down any other use cases, or even affect them in any way.
2. in parsing a roman text file, when processing a roman numeral symbol that
   • has a secondary roman numeral
   • is copied with the "=" syntax
     Only a small proportion of roman numeral symbols fulfill these conditions, and my PR makes the handling of roman numerals in romantext parsing consistent with elsewhere in the file (see the existing codebase here. It was not my decision to have roman numerals in romanText create new key objects (via deepcopy). (The comments below the line I have linked suggest that it is no slower than using a reference to the existing key, although I don't know why it should be so.) However, given that the parsing is implemented in this way, shouldn't it be consistent, so that issues like the transposition problem shown in my first post above don't arise? Note these commented out lines in the existing code:

            # surprisingly, not faster... and more dangerous
            # rn = roman.RomanNumeral(aSrc, kCurrent)

In any case, to address your concerns, I wrote the following brief script to compare the speed on a sample of roman text files from the When-in-Rome corpus. (It relies on the sh library and the fd shell utility so it will need to be rewritten if people want to run it themselves and don't have either of those.)

import random
from timeit import timeit
import sh

import music21

WHEN_IN_ROME_DIR = "/Users/malcolm/datasets/When-in-Rome/Corpus"

result = sh.fd("^analysis.txt", "--color", "never", WHEN_IN_ROME_DIR).stdout.decode().strip().split("\n")
# We need to exclude Beethoven quartets because of the issue at https://github.com/cuthbertLab/music21/issues/1412
result = [p for p in result if "Quartets/Beethoven" not in p]

seed = 42

random.seed(seed)
random.shuffle(result)

N_FILES = 50
N_ITERS = 2

sample = result[:N_FILES]

def f():
    for path in sample:
        print(f"parsing {path}")
        score = music21.converter.parse(path, format="romanText", forceSource=True)


print(timeit(f, number=N_ITERS))

The results (run 3 times on each branch) were as follows:

Existing implementation:
22.998510958
22.61021775
23.338384082999998

My rn-transpose branch:
23.126536791
22.741977666
22.842532917

It doesn't seem that my branch has caused a meaningful slowdown. In fact it's not obvious that there is a meaningful difference between the two branches.

[mscuthbert]

sounds good -- sorry i misinterpreted. I think that this has been fixed now? Closing - but reopen if I dreamt that I merged a PR.

[malcolmsailor]

Yes, you merged the PR. Thanks!