Switch to Symusic

[Natooz]

Following #112, this PR switches the MIDI load/write backend from miditoolkit to symusic.

@Yikai-Liao @lzqlzzq

[Natooz]

@Yikai-Liao things are moving, the test for one-track tok-detok are all passing!
Next step is multitrack, which should hopefully be quick. Then methods and misc. And I'll finish with the BPE which requires a few code modifications.

When everything is done on the MidiTok side, I'll try to contribute to symusic by adding a few tests on the CI pipeline, to ensure the robustness of the parsing/dumping processes.

[Natooz]

@Yikai-Liao we are almost done here!
There is still one small issue regarding the tempos, I'm not sur how the Tempo() should be created since the last update. It seems that the second positional argument initialises the MIDI int value for both the .qpm and .tempo attributes.

Hopefully when this is resolved we will be ready to review and merge!
This will mark the V3.0.0 of MidiTok (I'll still have a few things to do/fix before releasing it).
I'll run tokenization benchmarks in order to mesure the speed gains of the new version!

[Yikai-Liao]

@Yikai-Liao we are almost done here!
There is still one small issue regarding the tempos, I'm not sur how the Tempo() should be created since the last update. It seems that the second positional argument initialises the MIDI int value for both the .qpm and .tempo attributes.

Hopefully when this is resolved we will be ready to review and merge!
This will mark the V3.0.0 of MidiTok (I'll still have a few things to do/fix before releasing it).
I'll run tokenization benchmarks in order to mesure the speed gains of the new version!

Oh, I forget to change the tempo factory. I add qpm is to make it more clear that this property refers to quarters per minute， But I'm still not sure, should I use this abbreviation?

For the tempo factory, I think users should be able to choose one of These two arguments(microseconds per quarter and quarters per minute) as the argument of the constructor. Because in some kind of situations, we could just use qpm for convenience, but in other cases, we might need to avoid any precision loss by passing a int.

Two possible signatures might be

__call__(self, time, qpm=None, mspq=None)
# or
__call__(self, time, quarter_per_min=None, ms_per_quarter=None)

I'm not sure which one is better for users.

[Natooz]

I agree with you for letting the choice to the user.
I don't have strong opinions on the arguments names. Maybe the second proposition follows more common practices?

[Yikai-Liao]

@Natooz TempoFactory have been fixed Yikai-Liao/symusic#14

[Natooz]

Thank you!
I just tested it by installing from source, it should be good!
I still needed to manually check the tempos by rounding the qpm and mspq values instead of using the built-in __eq__ method, but that's ok as some info is lost with the conversions and there is nothing we can do about it.
When do you plan to release the next version?

[Yikai-Liao]

Thank you!
I just tested it by installing from source, it should be good!
I still needed to manually check the tempos by rounding the qpm and mspq values instead of using the built-in __eq__ method, but that's ok as some info is lost with the conversions and there is nothing we can do about it.
When do you plan to release the next version?

I only store mspq(int) in the struct. So I think currently there is no Information loss. So why do you need to define a custom __eq__ function?

Oh, are you considering the Information loss when creating tempo by qpm?

[Natooz]

Thank you for the clarifications!

Is there a reason to sort notes and pedals by end value?
Having the possibility to specify the keys directly in symusic would greatly reduce the overall tokenization time. But I don't want to ask more "secondary" features for symusic that would take some of your time again, as you already did so much.

[Yikai-Liao]

The reason is that Note is created when they are moved out of the last_note_on queue. And I just don't sort them. Its reasonable to sort them, may be by (start, duration) ?

if we only need to sort by attributes, we could directly generate (by macro ) $A_4^4 + A_4^3 + A_4^2 + A_4^1 = 24 + 24 + 12 + 4 = 64$ key functions. although it is quite stupid.

For example, we could write sort function like this:

sort(vec, key = "tpdv") {
    if(key == "tpqd"): 
        return sort_tpdv(vec)
}

But if we don't do it, we will need to write a simple jit strategy, which would be hard.

Although relection is possible solution, it seems quite tricky in c++. And I don't know how implement it.

[Natooz]

I'm not qualified to give advice on how to achieve it, but I'll just suggest to maybe sort by default by start/time, or maybe (start, pitch), as the pitch is an information carried at the same "time" or order than the onset/time of the note.
But I'll be happy to help if you need it!

[Natooz]

Preprocessing results from 56fdaef

Structured: 0.162 ± 0.173 sec
REMI: 0.172 ± 0.184 sec
MIDILike: 0.302 ± 0.323 sec
TSD: 0.174 ± 0.183 sec

Here the durations (of notes and pedals) are also preprocessed, by being aligned to the values of the tokenizer's vocabulary (except for MIDILike which is not concerned).
Sorting is done only by start by default, + pitch if using pitch intervals, + duration and velocity if using NoteOn/NoteOff tokens (MIDILike).

When omitting note sorting

Structured: 0.069 ± 0.057 sec
REMI: 0.074 ± 0.061 sec
MIDILike: 0.061 ± 0.050 sec
TSD: 0.075 ± 0.061 sec

[Yikai-Liao]

I have added sort for note and pedal while parsing midi. And the default sort key for NoteList and PedalList are changed to (time, pitch, duration) and (time, duration).

The new wheels (0.2.4) are building now.

[Natooz]

Wonderful!
Thank you, I'm testing it right away!

[Natooz]

@Yikai-Liao this is great!
The times of loading the MIDI + preprocessing is now on par with the last results measuring only the preprocessing without note sorting!

Structured: 0.071 ± 0.058 sec
REMI: 0.075 ± 0.063 sec
MIDILike: 0.062 ± 0.051 sec
TSD: 0.076 ± 0.062 sec

Thank you once again for your reactivity!

Concerning this PR, the switch is almost done, there is only this issue on Windows that I'll be fixing soon. When this is done, I think we will be reading to merge this (enormous) PR.
I added a few todos along the way, that should be done before releasing the v3.0.0. I'll still have some work to do, especially on the docs.

[Yikai-Liao]

utf8 file path support on Windows is added in librarify branch which will be merged to the main branch soon.

[Natooz]

@Yikai-Liao The bug happening on Windows was due to the § character in the file name of the MIDI being dumped. In MidiTok, this is done in the data augmentation method to tag the augmentation information in the file name.

It turns out that symusic raises an iostream error when trying to load a file with this character in the name.
Dumping a MIDI with § in the file name works, however the final file name ended altered (for example Aicha§v-4.mid to Aicha§v-4.mid in the example for the action here). I suspect this comes from an encoding error, § being not supported by the backend being used on Windows.

I simply switch from § to # in MidiTok, which solves the issue. Using § was purely arbitrary, I chose it as it is a character rarely used.
Anyway, I thought you might want to know this to try to make it work on all platforms. :)

I only have one task left before merging this PR! 🙌
And a few others after this before releasing the v3.0.0 hopefully in one or two weeks.

[Natooz]

I see our minds were synced here 🥲
I don't even have the time to report errors before you fix them 😄

[Natooz]

@Yikai-Liao @lzqlzzq
After extensive tests of almost all the codebase, I think the integration of symusic is finally done and well done. :)
We are good to merge this PR now, marking a nice way to end the year! 🙌
Thank you again for all your work and this great collaboration. 🫶 I truly believe symusic and minimidi are marking a big step in the community.

There are still some bugs and improvements that I want to do/fix before releasing the next release (v3.0.0). You'll be noticed when the changes will come.
Also, when the v3 will be released, as we can expect more people to use symusic, we can expect them to encounter some bugs. If that's the case, I'll keep you updated. Hopefully this should make the library even more robust and rich in features!