Fix BPM <-> MIDI tempo conversions #114

superbock · 2017-08-20T08:18:20Z

This PR addresses #102 and contains the fixes needed to correctly consider the time signature when converting between BPM (beats per minute) and MIDI tempo (microseconds per quarter note).

Before being able to be merged, these points should be discussed/addressed:

ticks_per_beat is a bit misleading since it rather should be ticks_per_quarter_note. Renaming the variable, however, may break existing code, so this is a rather bad option. Maybe adding a new variable (and deprecate the old one, but keep it around for backwards compatibility) is a better approach.
Tests for Python 2 fail at the moment since Python 2 and 3 round differently. This was revealed by adding tests for corner cases.
Extend the documentation to reflect all introduced changes, especially update the image in midi_files.rst.
Add more descriptive docstrings, but which docstring format should be used?

carlthome · 2017-09-29T19:05:28Z

mido/midifiles/units.py

@@ -17,7 17,7 @@ def second2tick(second, ticks_per_beat, tempo):
 note) and tempo (microseconds per beat).
 """
 scale = tempo * 1e-6 / ticks_per_beat
- return second / scale
+ return int(round(second / scale))


This change will break a program of mine where I let the user decrease the PPQ of MIDI files (for rendering them as piano rolls of reasonable dimensions).

ticks = int(round(mido.second2tick(message.time, ppq, tempo))) assert ticks > 0 or message.time == 0, "New PPQ is too low."

It would be good to see which part of the assert fails (I am not sure if this makes any difference, though), then it's easier to discuss this further.

The rationale behind this change is that "the MIDI standard" requires ~~QQN~~ PPQ to be an integer. And until now this function could return a float value.

I am not sure why ticks should be greater 0 or message timings should not be 0. Both are legitimate values as far as I can see. What's the intention behind these checks?

The problem seems to be that different Python versions handle the /-operator differently. Anyways, I see that this change breaks code (although I never thought it would), but IMHO the question remains if your assertion assumes the "right thing".

The rationale behind this change is that "the MIDI standard" requires QQN to be an integer. And until now this function could return a float value.

How should we handle the case when second will be truncated to zero for the given PPQ and tempo (or heavily shifted in time, ceil doesn't really help)?

What's the intention behind these checks?

The assertion checks that there will be at least one tick with the given ppq when message.time is larger than 0.0 seconds.

When PPQ is chosen too low there could be messages that don't move the tick count at all even though they should, which causes a slight global drift AFAIK.

The problem stems from MAPS having ridiculously high PPQ (craffel/pretty-midi#112), while just setting it to 960 is fine because no message actually seems to require that much time resolution to fit into a piano roll (although the messages might shift just slightly forward and/or backwards in time, which I don't bother to check).

Btw, what does QQN stand for?

Typo, PPQ = parts (ticks) per quarter note.

High PPQ (e.g. in the MAPS dataset) should be fine, since they result in large tick values. On the other hand, having low PPQ values is a (known) problem, but I am still not sure how this is affected by the proposed change.

The assertion checks that there will be at least one tick with the given ppq when message.time is larger than 0.0 seconds.

Ok, but I don't see how moving int(round(...)) inside the function should alter the result since you apply exactly the same rounding and casting to int by yourself. Are you sure that this change is the culprit and not something else?

Ok, but I don't see how moving the int(round(...)) thing inside the function should alter the result since you apply exactly the same rounding and casting to int by yourself.

Haha, you're absolutely right... Sorry. 😆

Could we have something like this internally maybe?

if second != 0 and tick == 0: warnings.warn(f"{second} seconds is zero ticks at {ppq} PPQ.")

carlthome · 2018-02-19T10:34:06Z

Status?

mgrachten · 2019-10-04T09:59:39Z

mido/midifiles/units.py

+ Depending on the chosen time signature a bar contains a different number of
+ beats. These beats are multiples/fractions of a quarter note, thus the
+ returned tempo depends on the time signature.


I think it is good to mention here that what constitutes a beat is defined by the denominator of the time signature, and give an example: in 2/4, 3/4, 4/4, ... the beat is a quarter note. In 3/8, 6/8, ... the beat is an eight note, etc.

mgrachten

I can confirm PR fixes the bpm2tempo, tempo2bpm, tick2second, and second2tick functions by factoring in the time signature. Previously the functions implicitly assumed the beat unit equals a quarter note.

Although I am not a collaborator on this project, I hope this review can contribute to the merging of the PR.

rdoursenaud · 2023-01-19T12:17:42Z

tests/midifiles/test_units.py

+ assert tempo2bpm(1000000) == 60
+ # x/4 time signature: 4 beats (quarter notes) per bar
+ # 60 bpm: 1 beat / sec = 1 quarter note / sec: 1 sec / quarter note
+ assert bpm2tempo(1000000, time_signature=(4, 4)) == 60


I believe this one and the following be tempo2bpm not bpm2tempo!

rdoursenaud · 2023-01-19T12:19:21Z

tests/midifiles/test_units.py

 assert second2tick(0.005, ticks_per_beat=100, tempo=500000) == 1
- assert second2tick(0.002, ticks_per_beat=100, tempo=100000) == 2
+ # TODO: Python 2 and 3 rounds differently, find a solution?
+ # The result produced by Python 3 seems the way to go


We are dropping Python 2 support for the next release. You can safely remove this TODO.

rdoursenaud · 2023-01-29T15:11:41Z

I have added the fix I was suggesting in #454 and merged your changes.

Thank you for your contribution!

superbock · 2023-01-29T15:25:15Z

Thanks a lot!

Closes mido#114

This was referenced Aug 20, 2017

Add different time units to MidiFile #115

Open

Classes need their parameters documented #111

Open

possible unit conversion bug #102

Closed

superbock force-pushed the fix_unit_conversions branch from 92290e1 to 9a18de8 Compare August 23, 2017 08:50

superbock force-pushed the fix_unit_conversions branch 2 times, most recently from 9adf766 to a4678d1 Compare September 27, 2017 12:26

carlthome reviewed Sep 29, 2017

View reviewed changes

superbock mentioned this pull request Oct 16, 2017

[WIP] replace old MIDI code in utils.midi with mido CPJKU/madmom#308

Closed

2 tasks

superbock force-pushed the fix_unit_conversions branch from a4678d1 to 4008d43 Compare December 14, 2017 15:06

Sebastian Böck added 2 commits March 4, 2018 14:52

consider time signature when convertig between BPM and MIDI tempo

19651b7

second2tick() returns integer values only

477d553

superbock force-pushed the fix_unit_conversions branch 2 times, most recently from 58b87fd to 83caf8d Compare March 4, 2018 14:28

Sebastian Böck added 2 commits March 4, 2018 15:31

clarify docstring for tick2second() and tick2second()

d38a848

update docs to include time signature relevance for event timings

c086918

superbock force-pushed the fix_unit_conversions branch from 83caf8d to c086918 Compare March 4, 2018 14:31

mgrachten reviewed Oct 4, 2019

View reviewed changes

mgrachten approved these changes Oct 4, 2019

View reviewed changes

This was referenced Feb 20, 2022

MIDI file duration too long / rhythm is too slow #351

Open

timing issue – elapsed time not matching mid.length? #359

Open

rdoursenaud added the feature:midifile Standard MIDI File (SMF) implementation label Jan 19, 2023

rdoursenaud requested changes Jan 19, 2023

View reviewed changes

rdoursenaud added this to the Version 2.0.0 milestone Jan 19, 2023

rdoursenaud self-assigned this Jan 29, 2023

rdoursenaud merged commit d816bde into mido:main Jan 29, 2023

georgeharker pushed a commit to georgeharker/mido that referenced this pull request Jun 12, 2024

Fix tests and clarify tempo2bpm

f8ce73f

Closes mido#114

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BPM <-> MIDI tempo conversions #114

Fix BPM <-> MIDI tempo conversions #114

superbock commented Aug 20, 2017

carlthome Sep 29, 2017 •

edited

Loading

superbock Sep 29, 2017 •

edited

Loading

carlthome Oct 2, 2017

carlthome Oct 2, 2017

superbock Oct 2, 2017 •

edited

Loading

superbock Oct 2, 2017 •

edited

Loading

carlthome Oct 2, 2017 •

edited

Loading

carlthome commented Feb 19, 2018

mgrachten Oct 4, 2019

mgrachten left a comment

rdoursenaud Jan 19, 2023

rdoursenaud Jan 19, 2023

rdoursenaud commented Jan 29, 2023

superbock commented Jan 29, 2023

Fix BPM <-> MIDI tempo conversions #114

Fix BPM <-> MIDI tempo conversions #114

Conversation

superbock commented Aug 20, 2017

carlthome Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

superbock Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

carlthome Oct 2, 2017

Choose a reason for hiding this comment

carlthome Oct 2, 2017

Choose a reason for hiding this comment

superbock Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

superbock Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

carlthome Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

carlthome commented Feb 19, 2018

mgrachten Oct 4, 2019

Choose a reason for hiding this comment

mgrachten left a comment

Choose a reason for hiding this comment

rdoursenaud Jan 19, 2023

Choose a reason for hiding this comment

rdoursenaud Jan 19, 2023

Choose a reason for hiding this comment

rdoursenaud commented Jan 29, 2023

superbock commented Jan 29, 2023

carlthome Sep 29, 2017 •

edited

Loading

superbock Sep 29, 2017 •

edited

Loading

superbock Oct 2, 2017 •

edited

Loading

superbock Oct 2, 2017 •

edited

Loading

carlthome Oct 2, 2017 •

edited

Loading