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

Incorrect Placement of Time Signatures #131

Closed
EterDelta opened this issue Jan 19, 2024 · 1 comment · Fixed by #132
Closed

Incorrect Placement of Time Signatures #131

EterDelta opened this issue Jan 19, 2024 · 1 comment · Fixed by #132

Comments

@EterDelta
Copy link

Recently, I've been testing the new version 3.0.0, particularly with MIDIs that feature multiple time signature changes.

There seems to be a regression related to the time_signature_range parameter. An issue arises when supporting more time signatures than those present in a song. In this case, the time signatures are incorrectly placed.

Steps to Reproduce

  • Instatiate any tokenizer that supports time signatures. (I've tested with REMI+ and TSD)
  • Configure it to support all the song time signatures and an extra one.
cfg = TokenizerConfig(
    use_tempos=True,
    use_programs=True,
    use_time_signatures=True,
    time_signature_range={4: [7, 4, 2], 8: [2]} # Here, 2/8 being the one not present in the song.
)
  • Process the song as normal and dump a file of it.
tokens = tokenizer("example_song.mid")
sequence = tokenizer(tokens)
sequence.dump_midi("example_song_processed.mid")
  • Open the output file in any MIDI viewer, and compare with the original.

As you can see, the first signature (2/2) in the original song lasts 2 beats, but in the processed file, it now lasts for 4.
If we remove the extra signature from the configuration, the problem is fixed.

This issue is also present in different signatures and songs.

example_song.zip

@Natooz
Copy link
Owner

Natooz commented Jan 19, 2024

Hello, 👋

Thank you very much for the bug report! 🙏
It's fixed in #132, I'll wait for the tests to pass and merge it!
You then will be able to get the fix by installing from git: pip install -U git+https://github.com/Natooz/MidiTok

The error came from the time division used in the time signature preprocessing method. It used the tokenisers one (which is used when decoding tokens for example) instead of the MIDi's one, which can be different, resulting in the time signature shifts.

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 a pull request may close this issue.

2 participants