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

Improve conversions between ticks and seconds #531

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rdoursenaud
Copy link
Member

Deprecates tick2second in favor of ticks2seconds and second2tick in favor of seconds2ticks.
Returns fractional time is seconds per #528
Removes erroneous references to absolute in docstrings and comments.

Fixes #526

Deprecates tick2second in favor of ticks2seconds and second2tick in
favor of seconds2ticks.
Returns fractional time is seconds per mido#528
Removes erroneous references to absolute in docstrings and comments.

Fixes mido#526
@rdoursenaud rdoursenaud added this to the Version 2.0.0 milestone Aug 4, 2023
@rdoursenaud rdoursenaud self-assigned this Aug 4, 2023
@page200
Copy link

page200 commented Aug 4, 2023

Thank you for the nice discussions and commits! :)

One comment: I'd favor renaming resolution to the units it is given in, such as ticks_per_quarternote.

@rdoursenaud
Copy link
Member Author

The unit is a property that is not descriptive enough IMHO.

I find that resolution conveys the intention better and I intend to use the same field name when revamping the MIDIFile class.

I pondered using time_ticks, time_seconds, resolution_tpqn and tempo_uspqn but I think it makes the API overly verbose and may confuse users more because of the lack of context provided by the documentation.

@rdoursenaud
Copy link
Member Author

And thanks for the review!

@page200
Copy link

page200 commented Aug 4, 2023

In what way do you consider ticks_per_quarternote not descriptive enough, I mean, what variables except for resolution can be given in ticks per quarternotes? If there are such variables, let's call this one resolution_in_ticks_per_quarternote or so. I suspect that ticks_per_quarternote is a good choice. Informative names are good.

Leaving out units would be a huge pain, especially in the context of MIDI, where time is so complicated already due to ticks vs. seconds vs. quarternotes and the various events that affect how to convert between those "units". "Resolution" might be given in ticks per quarternote, or ticks per second, or ticks per millisecond, or ticks per minute, or ticks per bar. Or maybe other things can also be considered "resolution", for example something related to the limited numerical precision of the inexact number of seconds that users might assume or recall from before the fix of #528, or the limited numerical precision of data in MIDI events such as tempo changes, or an assumption that mido voluntarily chooses to limit the resolution to some value by rounding. All these units, conversions, MIDI events (including how they affect conversions over time and how they have limited precision) and optional roundings are dauting, and ticks_per_quarternote nails it, it seems.

@rdoursenaud
Copy link
Member Author

ticks_per_quarternote is just a unit it doesn't convey its purpose.
I just checked the SMF specification and they call this division, maybe we should use this to avoid bikeshedding as it is the trusted source.

@rdoursenaud
Copy link
Member Author

rdoursenaud commented Aug 4, 2023

Also the division field will need to have an intrinsic unit anyway since it can either be expressed in ticks per quarter note or ticks per frame.
We don't have support for the latter at the moment. See #457.

@rdoursenaud rdoursenaud added the feature:midifile Standard MIDI File (SMF) implementation label Aug 6, 2023
@rdoursenaud rdoursenaud marked this pull request as draft August 7, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature:midifile Standard MIDI File (SMF) implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong code comments about relative and absolute time?
2 participants