Skip to content
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

Handle endings in TSV -> music21 conversion #1503

Merged
merged 9 commits into from
Dec 29, 2022

Conversation

malcolmsailor
Copy link
Contributor

In a #1412 (comment), I proposed the following two options for resolving the issue of 1st and 2nd endings being written to the same measure in TSV conversion:

  1. insert ending brackets for the 1st and 2nd endings (based on the volta column present in the DCML tsv files) but without any associated repeat bars---this is what I already more or less implemented.
  2. put the 1st (or 1st and 2nd, if there is a 3rd ending, etc.) ending into some sort of metadata as I suggested above.

In discussion at MarkGotham/When-in-Rome#58 (comment), @MarkGotham expressed a preference for option 1, and no one else seemed to express a preference, so that's what this PR implements.

One remaining issue (raised in the comments of my code) is

# Should we warn the user that, although we're writing
# repeat brackets, we aren't writing repeat signs since
# the .tsv file doesn't tell us where the forward repeat
# should be?

@coveralls
Copy link

coveralls commented Dec 17, 2022

Coverage Status

Coverage increased (+0.001%) to 93.048% when pulling 2dd6168 on malcolmsailor:tsv_endings into cddb104 on cuthbertLab:master.

@MarkGotham
Copy link
Contributor

Great thanks @malcolmsailor !

Wrt repeat signs, are we talking about the inclusion of :|| in a case like:

m2a V :||
m2b V

Surely we want that included? I don't see what'd be gained by omitting it.

Apologies if I've got the wrong end of the stick!

@malcolmsailor
Copy link
Contributor Author

Yes, I do mean omitting that repeat bar. Because we don't know where the corresponding forward repeat bar would be, so including it would lead to incoherent repeats.

Moreover we don't necessarily know where the first ending repeat-bar should go either. For example, consider a TSV looking something like this

mn  chord   volta
1   I             
2   V       1
2   I       2
4   I

This is ambiguous between what could be written in romanText as

m1 I
m2a V :||
m2b I
m3 I
m4 I

and

m1 I
m2a V
m3a V :||
m2b I
m3b I
m4 I

Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but see note on typing. If the TSV team would be willing to add mypy typing for the module, it'd help me from not needing to do it later (getting frustrated, etc., because I don't know the code well, etc.) -- it mostly means not allowing "None" for variables that are clearly meant to be strings.

@@ -410,6 +407,7 @@ def __init__(self):
super().__init__()
self.mn = None
self.mn_onset = None
self.volta = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mentioning this because it's an addition, but it's the same for all of these. It would be better if the default for these was not None but '' because this sets the typing of the usage later. We're heading towards > 90% typing in mypy so it would be very helpful if these could be typed and if the typing was not str | None but just str which requires a '' default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice for instance, that you're not doing "and thisChord.volta is not None" but instead just "and thisChord.volta" so setting the default to an empty string won't cause a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK added some type hints in a new commit. Also initialized to the empty string where that didn't cause any hassle.

@mscuthbert
Copy link
Member

looks good to me. Merging. Happy New Year!

@mscuthbert mscuthbert merged commit 9e87495 into cuthbertLab:master Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants