-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
romanText/tsvConverter.py update work in progress / coming soon #1214
Comments
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. |
Sure thing. Probably the best approach is to set these numbers in relation the DCML version (previously 1.0; now 2.0)? |
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 |
So would the best approach for the TSV converter be something like this then?
|
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. |
Thanks @malcolmsailor! For the handling of 6th and 7th degrees in minor, see the |
There was another problem that I believe I fixed though: the 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. |
Thanks @malcolmsailor . Couple of tips here:
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. |
I used both the sixthMinor and seventhMinor arguments, setting them both to |
I believe the reason for this is that
I will try using |
OK, Mark, I made a pull request. If you want to get the commented-out test running you are more than welcome. |
Haven't looked in detail, but on the basis of prior discussions and PRs like #1230, I thought >>> 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> |
Thanks, @jacobtylerwalls. In fact the issue actually turned out to have been that the code used @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 |
Discussion wrt this topic is advancing in #1267, so this issue can safely be closed. |
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:
The text was updated successfully, but these errors were encountered: