cuthbertLab / music21

music21 is a Toolkit for Computational Musicology
https://www.music21.org/
Other
2.09k stars 397 forks source link

romanText/tsvConverter.py update work in progress / coming soon #1214

Closed MarkGotham closed 2 years ago

MarkGotham commented 2 years ago

For information and to invite music21-related comments and suggestions:

The DCML standard for harmonic analysis has changed, so an update to the music21/romanText/tsvConverter is needed.

Here are the relevant links for discussion / work in progress:

mscuthbert commented 2 years ago

I hope the new standard incorporates a version number. Older files will still be out there and need to be parsed in the old way.

MarkGotham commented 2 years ago

Sure thing.

Probably the best approach is to set these numbers in relation the DCML version (previously 1.0; now 2.0)?

johentsch commented 2 years ago

Thanks for the initative, @MarkGotham and @malcolmsailor! We'll be happy to provide support and assistance if required! This could be an important step for making the standards interoperable for everyone.

Yes, @mscuthbert, the DCML standard is versioned and the employed version is indicated in the annotations' metadata (a promise yet to be filled for ABC and MPS in their soon-to-follow updates). Once ABC v2 will finally be up and running in the near future, the "old way" will actually be deprecated. From that point, the latest regEx will be downward-compatible with all current and future 2.x.x annotations.

malcolmsailor commented 2 years ago

So would the best approach for the TSV converter be something like this then?

malcolmsailor commented 2 years ago

I implemented a flag allowing to switch between v1 and v2 parsing here. Before I submit a pull request, one question:

I implemented a test where we convert to music21, then back to DCML, then back to music21 again, then compare the streams to ensure they are the same.

However, this test fails because vii (and possibly other roman numerals) seem to be treated differently going forward and going back: "#viio7" in a DCML file becomes "viio7" in music21, which is then written back out as "viio7" and then, when read in anew, becomes "bviio7". So I'm wondering if music21 provides any utilities that will allow us to write out "viio7" as "#viio7" and similar.

MarkGotham commented 2 years ago

Thanks @malcolmsailor! For the handling of 6th and 7th degrees in minor, see the Minor67Default options in roman.RomanNumeral. E.g. sixthMinor=Minor67Default.QUALITY. I think I that's not integrated into the v1 DCML converter because the converter was written before the Minor67Default was implemented. In any case, that's clearly the way to go now.

malcolmsailor commented 2 years ago

sixthMinor=Minor67Default.QUALITY appears to be the default when the RomanNumeral class is instantiated. I tried changing QUALITY to both FLAT and SHARP and either way the test still fails in the same way.

There was another problem that I believe I fixed though: the M21toTSV class used the romanNumeralAlone attribute of the RomanNumeral to write out the figure (I did not write this code, although I did adapt it to DCML v2), and according to the docs this strips accidentals from the roman numeral, so it will never write out something like '#vii'. So instead I am matching a regex to RomanNumeral.primaryFigure to get the roman numeral with its accidental (but without any figures).

I'm not inclined to spend any more time trying to fix this issue at the moment since it was already there before I implemented the v2 conversion (although it may have been unnoticed) and, as far as I'm aware, no one is actually performing conversion in this direction.

MarkGotham commented 2 years ago

Thanks @malcolmsailor .

Couple of tips here:

  1. Perhaps I wasn't clear before. Apologies. I mention sixthMinor to introduce this issue and the .QUALITY etc options generically. More specifically, sixthMinor is for scale degree 6. There's a seventhMinor for degree 7 and that's the one you want for the case of vii.

  2. Yes, romanNumeralAlone sounds like the wrong attribute here and set to make all the above moot. Thanks for spotting that. If RomanNumeral.primaryFigure works in your solution then great. For information there's also RomanNumeral.romanNumeral that returns the figure with no inversion info. (unlike .figure) but with leading accidental (unlike romanNumeralAlone).

If you're moving on then thanks for your work to date. Please get the current version stable for a PR. Then I or someone else can take it from there. It'd be a shame to leave a known error in.

I don't know whether anyone is using the m21-DCML currently, but I can say that there's a lot more talk and active plans wrt coordination and interoperability at the moment (as @johentsch hints in his comments above), so this would seem a tool worth getting right.

malcolmsailor commented 2 years ago

I used both the sixthMinor and seventhMinor arguments, setting them both to QUALITY, SHARP, and FLAT.

malcolmsailor commented 2 years ago

I believe the reason for this is that primaryFigure seems to omit the leading sharp.

>>> from music21.roman import RomanNumeral as RN
>>> from music21.roman import Minor67Default as sixsev
>>> RN("viio", "c", seventhMinor=sixsev.QUALITY).romanNumeral
'#vii'
>>> RN("viio", "c", seventhMinor=sixsev.QUALITY).primaryFigure
'viio'

I will try using romanNumeral.

malcolmsailor commented 2 years ago

OK, Mark, I made a pull request. If you want to get the commented-out test running you are more than welcome.

jacobtylerwalls commented 2 years ago

I used both the sixthMinor and seventhMinor arguments, setting them both to QUALITY, SHARP, and FLAT.

Haven't looked in detail, but on the basis of prior discussions and PRs like #1230, I thought CAUTIONARY might have been the answer. Is it?

>>> from music21.roman import RomanNumeral as RN
>>> from music21.roman import Minor67Default as sixsev
>>> RN("viio", "c", seventhMinor=sixsev.CAUTIONARY)
<music21.roman.RomanNumeral viio in c minor>
malcolmsailor commented 2 years ago

Thanks, @jacobtylerwalls. In fact the issue actually turned out to have been that the code used romanNumeralAlone (which omits the leading accidentals) rather than romanNumeral.

@MarkGotham, in the latest commit I believe have fixed the issue mentioned in my last few comments. After repairing the leading-accidental issue, the remaining issue seems to have been that the m21 to tsv conversion was not writing the form and figbass columns, which the tsv to m21 conversion used to reconstruct the chord. I implemented writing out those columns in m21 to tsv conversion. The test in question (where we do tsv -> m21 -> tsv -> m21 and then compare the m21 streams) now passes.

MarkGotham commented 2 years ago

Discussion wrt this topic is advancing in #1267, so this issue can safely be closed.