Skip to content

Respect quantizePost=False when parsing raw MIDI#548

Merged
mscuthbert merged 7 commits into
cuthbertLab:masterfrom
jacobtylerwalls:raw_midi_no_quantize
May 31, 2020
Merged

Respect quantizePost=False when parsing raw MIDI#548
mscuthbert merged 7 commits into
cuthbertLab:masterfrom
jacobtylerwalls:raw_midi_no_quantize

Conversation

@jacobtylerwalls
Copy link
Copy Markdown
Member

@jacobtylerwalls jacobtylerwalls commented May 27, 2020

Fixes #546, fixes #192 and adds test method converter.testParseMidiNoQuantize()

New example MIDI file containing a 1/4 measure with exactly 3 16th notes and 2 32nd notes

@coveralls
Copy link
Copy Markdown

coveralls commented May 27, 2020

Coverage Status

Coverage increased (+0.03%) to 90.693% when pulling 9677449 on jacobtylerwalls:raw_midi_no_quantize into 850a91a on cuthbertLab:master.

@mscuthbert
Copy link
Copy Markdown
Member

Closing and reopening to rerun tests since there were potentially MIDI keyword conflicts with a recently merged PR.

@mscuthbert mscuthbert closed this May 29, 2020
@mscuthbert mscuthbert reopened this May 29, 2020
@mscuthbert
Copy link
Copy Markdown
Member

Looks really great, but couple of comments:

  • forceSource and not use pickle slow down the test suite considerably. Have you gotten the parsing down to the minimum level of using forceSource to not cause a problem? I imagine that only the skipping quantize parse needs it.

  • How will anyone know that they can pass in this new keyword? Documentation needs to be updated as well. Don't worry about the User's Guide, but at least the MIDI Converter and a few other places need it.

Thanks!

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

Sure thing, will definitely get on the documentation. With luck it could cut down inquires about 32nd notes.

RE: pickles -- so long as I am testing the same file and parsing it two ways, I think I would have to force the source each time. Otherwise if the first call is forceSource=False, quantizePost=True, upon running the test a second time, wouldn't it just load in the result of the second parsing?

The motivation was to avoid a false positive test result if behavior changed elsewhere, but I could check for that more directly by just asserting that every value in defaults.quantizationQuarterLengthDivisors is less than 8, so if that behavior ever changes then this test will complain that it must be rewritten. Then I could just call parse once and let it use a pickle.

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

jacobtylerwalls commented May 30, 2020

Fixes #192 (documentation request)

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

On second thought I definitely still need to forceSource even when parsing the test file one time, otherwise when the test passes once it will pass forever -- need to ensure I'm testing that the pipeline for **keywords remains intact all the way down from converter.parse()

@jacobtylerwalls
Copy link
Copy Markdown
Member Author

• Noticed MIDI file had an incorrect note_off value that was causing M21 to fill a gap with an extraneous rest. Didn't affect my tests, but want to contribute a clean test file.

• Noticed two more places to document the quantization keywords.

@mscuthbert
Copy link
Copy Markdown
Member

Thanks -- good explanation for why you need forceSource in both these cases. Thanks for trying it. Will review new changes today!

@mscuthbert
Copy link
Copy Markdown
Member

This is a model PR. Thank you Jacob! Will definitely make MANY people happy!

@mscuthbert mscuthbert merged commit daad838 into cuthbertLab:master May 31, 2020
@jacobtylerwalls jacobtylerwalls deleted the raw_midi_no_quantize branch June 1, 2020 12:40
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.

Let MIDI converter read keywords when parsing raw data Provide easy way to disable MIDI quantization

3 participants