Skip to content

Conversation

@TimFelixBeyer
Copy link
Contributor

@TimFelixBeyer TimFelixBeyer commented May 21, 2023

This PR adds functionality to parse <sound tempo='x'> tags from MusicXML files into MetronomeMark objects (fixing #1578).
bach/bwv66.6 contains such a <sound tempo='x'> tag and is heavily used in doctests. Therefore a lot of tests had to be updated, leading to quite a lot of changes, however nothing seems to be broken.
The only tests that had significantly different outputs are related to feature extraction.
They change because the changed tempo leads to a (very) different beat histogram.
Previously, the tempo was 120bpm, leading to beat bpm values of 60, 120, 240, where 240 was discarded due to the this line:

if thisBPM < 40 or thisBPM > 200:

The accurate tempo is 96, so the values are 48, 96, 192 and are therefore no longer discarded.

Let me know what you think!

@coveralls
Copy link

coveralls commented May 21, 2023

Coverage Status

Coverage: 93.115% (+0.009%) from 93.105% when pulling 18fdfd6 on TimFelixBeyer:patch-8 into 2f6833a on cuthbertLab:master.

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.

Approved! This is a huge improvement. Thank you! I had no idea there were so many XML files with tempos specified in <sound> elements but without metronomes. I'm going to close and re-open to see if there are any conflicts with the stripTies bug fix.

@mscuthbert mscuthbert closed this May 27, 2023
@mscuthbert mscuthbert reopened this May 27, 2023
@mscuthbert
Copy link
Member

Indeed - - the typing did catch a minor bug and added a warning that something that appeared typed was not. Thanks for being totally on top of this!

Import tempo as int if possible, allowing better braille display of metronomes
removed a `<sound tempo="0">` tag in schoenberg/opus19 to fix warnings
@TimFelixBeyer
Copy link
Contributor Author

TimFelixBeyer commented May 27, 2023

A file in the corpus (schoenberg/opus19) had an incorrect tag, I have removed it.
In addition, I have changed the existing way metronomes were imported to prefer ints over floats where possible to allow for better braille compatibility.

@mscuthbert
Copy link
Member

It also just looks more musical to see qtr=144 than 144.0, and
more accurate to the composer's notation. So good!

@mscuthbert
Copy link
Member

Sorry for the delay in merging -- I just needed to check out the Schoenberg file and make sure all was good.

@mscuthbert mscuthbert merged commit e25334b into cuthbertLab:master May 27, 2023
@TimFelixBeyer TimFelixBeyer deleted the patch-8 branch May 28, 2023 01:31
@mscuthbert
Copy link
Member

I'm changing this a little bit in the next PR because I'm seeing phantom metronome marks in lots of pieces that should not show them in scores (like medieval/Renaissance music, Bach Chorales, etc.) -- these only set numberSounding so that the metronome mark does not appear in the score, just affecting playback.

jacobtylerwalls added a commit that referenced this pull request Jun 10, 2023
Not used in the implementation from #1579
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.

3 participants