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

Add a faster midi parsing backend #112

Closed
Yikai-Liao opened this issue Nov 27, 2023 · 59 comments
Closed

Add a faster midi parsing backend #112

Yikai-Liao opened this issue Nov 27, 2023 · 59 comments
Labels
enhancement New feature or request

Comments

@Yikai-Liao
Copy link

Yikai-Liao commented Nov 27, 2023

Recently, I have writen a lightweight midi parsing library (writen in c++ with pybind11), wich is significantly faster than miditoolkit.

https://github.com/Yikai-Liao/symusic

Midi parsing speed has been the bottleneck of preprocessing for a long time, and I want to deal with this problem.

Please tell me if you need any help or if i need add new features to the library. (Note that not all MIDI events are supported currently. For example, we ignored lyrics and pitch bend. Wel will add these features in the future if you need them. )

Here is the midi parsing benchmark:

  • test using mahler.mid from minimidi/example in colab
  • mido is writen in pure python, and only parses midi files to event level
  • pretty_midi and miditoolkit is based on mido, and parse midi files to note level
libarary time
symusic 21.8 ms ± 11.7 ms
MIDI.jl 128.024 ms
mido 5.68 s ± 2 s
pretty_midi 5.59 s ± 844 ms
miditoolkit 6.27 s ± 1.79 s
music21 8.59 s ± 1.2 s
@Natooz
Copy link
Owner

Natooz commented Nov 27, 2023

This looks promising!
I'll work on a current PR right now and dig deeper into this when it's done. :)
Now from what I just red, I think to use it in MidiTok it would require to support writing files, and load other event types. MidiTok supports Pedals, Pitch Bends, Tempo changes, Time Signature changes. Ultimately we plan to also include Control Changes, and it would be great if one day we can also (allow users to) embed everything else such as Key Signature or Lyrics / Markers somehow.

Also, the way I saw the switch was to use the new c++ backend by default when reading / loading files, but still supporting miditoolkit.MidiFile objects for a while by converting them to the new backend Python class. The conversion would be done in MidiTok, but that's just to say to the new backend should allow to manipulate the data and create an object from scratch

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Nov 27, 2023

That's great. It's easier to support other types of midi events, while writing back will lead to heavy workload. I'll try to implement these functions.

And is there any high level interface you need for better integration? Currently, we merely add some functions like time_shift, clip, sort, to meet our own needs, and the pianoroll conversion function is quite casual.

We are trying to make a more friendly interface in python level.

@Natooz
Copy link
Owner

Natooz commented Nov 27, 2023

It's easier to support other types of midi events, while writing back will lead to heavy workload. I'll try to implement these functions.

I totally get it. :) I didn't touch c/c++ in a long time, but if there is a way I can help don't hesitate to tell me!
Until writing is implemented, I can open a PR here to first handle the loading/parsing, and mock writing with miditoolkit (which is a very small part).

And is there any high level interface you need for better integration? Currently, we merely add some functions like time_shift, clip, sort, to meet our own needs, and the pianoroll conversion function is quite casual.

For MidiTok, the needs are mainly to parse the MIDI data with respect to the General MIDI 2 specifications, so I guess you already cover them well. 🙌
I'll give it a try soon, and compare it with miditoolkit (make sure the content is loaded exactly the same).

@Natooz
Copy link
Owner

Natooz commented Nov 28, 2023

I tried Symusic, it would be great if MidiTok can benefit from such speed!

Now I have some questions, more towards how you envision Symusic, to know if it can fit with MidiTok.
For the time unit, I saw that you define times in "quarter note". I suppose this is because you must have some use for this. But would you consider to use ticks (integers) as the default time unit ? And maybe add some method to convert the time unit across all the events of a MIDI into quarter notes / beat / seconds?
As the tick is the default unit, it would allow users to have a greater flexibility over the time dimension, for any downstream usage.

I checks the note loading, they seems to be identical to what is loaded with miditoolkit, at least I found the same number of notes, and the notes I manually checks contained the same data except when they are not sorted exactly the same but that's not an issue.

A few suggestions/comments:

  • Add an attribute in Score for the MIDI's time division (ticks per beat);
  • I don't know the reason for naming this "Score", but maybe "MidiFile" would be more accurate :) ;
  • Set Note.duration not as an attribute but as a property calculated as self.end - self.start, that would remain correct if one of these two attributes is modified;
  • I got an error when iterating over Score.tracks when the TrackList is empty;
  • Do you think it is possible to provide more type hints, and debugging exploration of the MIDI attributes? It would greatly ease debugging when programming. Below is an example of what can be shown with miditoolkit and the PyCharm debugger.
Capture d’écran 2023-11-28 à 15 39 47

Overall I think this is promising, the speed gain are amazing. I don't know if I can help much on the C++ side, but maybe I can do something for the Python binding part.

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Nov 29, 2023

Thanks for your feedback. I'll answer some of the questions, while @lzqlzzq (the author of minimidi, also the co-author of symusic) will answer the others, especially those about architecture.

  1. For Note.duration, I actually store duration in the class, not the end time. So end could not be modified. Here, I just think both choices (store duration or end time) are acceptable, and I choose one of them casually. Is there any advantages for choosing a specific one?

     class Note {
     public:
         float start;
         float duration;
         int8_t pitch;
         int8_t velocity;
     }
  2. I'll try to fix the errors about empty track list. We have met many errors caused by empty during developing this library. Many thanks for the test.

  3. For the type hints and debugging information (actually it is the string got from repr functions)

  • I have generated a symusic.pyi file for type hints. And I have noticed that it can't work well with ipython. So is there any methods for offering better type hints?
  • As for the repr function, I found it too verbose especially when the List is long, like[Note(start=xxx, end=xxx, pitch=xxx, velocity=xxx), Note(start=xxx, end=xxx, pitch=xxx, velocity=xxx)...] . However, just like what you thought, just offering length information is also not a good design for debugging. I think the idealized implementation might be that in pandas, which is hard to achieve. So I wonder if there is a more suitable design (if not, I'll just rollback to the format in miditoolkit) ?

By the way, I have added Marker, Lyrics, and Pitch Bend to symusic (haven't upload to pypi) and minimid, but I have some problems with Pedal. I have opened an issue in miditoolkit. Do you have any ideas about this?

@lzqlzzq
Copy link

lzqlzzq commented Nov 29, 2023

Thanks for your attention and feedback on symusic~
Here are some of our design considerations and developing processing:

  1. For the time unit, we think "quarter note" is a unified unit compared to MIDI tick. In MIDI files, I have seen 960, 320, 192 ticks per quarter as far as I know. A unified unit is more consistent in semantics, quarter in float is suitable. For the flexibility and usability, we will consider add MIDI tick and second maybe as attributes.
  2. The name Score is the alias of sheet music. We consider the Score as a intermediate representation of symbolic music rather than just MIDI, because musicxml, ABC and other format can be converted into the Score while retaining most of original information.
  3. There is an advantage using start, duration rather than start, end to store the time information of a note: when performing a time_shift on N notes, using start, duration only needs N additions, while using start, end needs 2N. And I didn' t see any visible advantage of using start, end.
  4. Would you please provide the MIDI file you used to reproduce the TrackList is empty problem? That will help us locate the problem.
  5. We are still looking into the type hint issue, because making type hint in a pybind project is tricky. :(

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Nov 29, 2023

Additionally, the conversion between "quater note" and MIDI tick is quite simple, while the conversion between seconds and MIDI tick is quite complex (also, using second as time unit will make it hard to reset bpm for a specific range of time).

Of course, this kind of conversion will cause a certain degree of precision loss.

from miditoolkit import MidiFile
midi = MidiFile("midi file path")
ticks_per_quarter = midi.ticks_per_beat
# conversion
start_midi_tick: int = midi.instruments[0].notes[0]  # start time of an example note
start_quarter: float = start_midi_tick / ticks_per_quarter 
# For writing back, we just need to set a new quantization factor, the same meaning as ticks per quarter
q = 960 # for example
start_quantized = int(start_quarter * q)

@Natooz
Copy link
Owner

Natooz commented Nov 29, 2023

Hi guys, thank you for the quick feedback!

I finally got time to run a more complete "benchmark", here is the script: https://gist.github.com/Natooz/1a39413220d038e0ac3981f864039370
I run it on the test cases of MidiTok.
Here are the results, run on a i7 6700k:

POP909_191.mid: 105 errors (0.05)
POP909_008.mid: 156 errors (0.09)
POP909_022.mid: 13 errors (0.01)
POP909_010.mid: 127 errors (0.08)
+----------------------------------+
|  MIDI loading speed comparison   |
+-------------+--------------------+
|     Lib     |       Speed        |
+-------------+--------------------+
| MidiToolkit | 0.7407 ± 0.750 sec |
|   SyMusic   | 0.0014 ± 0.001 sec |
+-------------+--------------------+

The good news is that almost all the notes are retrieved exactly the same, with a few exception after time signature changes. Here are the saved files. failed_midis.zip
And of course, the speed boost, 529 times faster in average here 🥲🥲

I'll answer to each points respectively now.

@Yikai-Liao

  1. Note.duration: Great, I though the two were "stored" as attributes. I actually don't see any advantage of storing one over the other, I think both are convenient, as long as one is stored and the other computed on the fly. :)
  2. If I have time, I'll try more elaborate tests;
  3. Type hints: I don't know methods that can provide such "interface", I which I had some knowledge on Python bindings. But this might be a good start, I'll maybe have some more free time from the period from January to March to look into it. I put this suggestion, but I don't think this is something urgent or of high priority, it's just debugging convenience for users.
  4. Marker, Lyrics, and Pitch Bend: Great, I'll inspect the PR very soon (maybe tonight, otherwise tomorrow I hope).

@lzqlzzq

  1. Noted. I still suggest to adopt the tick as default unit, and add attributes for the beat equivalent, that could be chosen to be computed or not when parsing the MIDI. The reasons are that the tick remains the native unit, and can be useful/required in many use cases. It keeps 100% of the original time information with no approximations, which is quite useful in the case of MidiTok as we use it to downsample the starting and ending time of the events. It also allows to performs computations with integers instead of floats, which can be significant in Python with large files. Even if we adopt Symusic, we would have to reconvert back all "beat" times into ticks, in Python. I assume other users of Symusic (for any task) would be in a similar situation. Now for the seconds, that's another story. I personally don't have usage of this, but users of pretty_midi do, for transcription for example. I guess supporting conversions to seconds in Symusic would attract them :) ;
  2. I got your point;
  3. I agree!;
  4. Its the empty.mid files in the test cases;
  5. That's great! For my own usage, I don't think this would be a big issue, but I guess that having appealing interfaces would convince users to adopt Symusic and minimidi.

Another thing, while writing the benchmark I had to manually recreate the lists of notes from the tracks in order to sort them.
I guess the *List classes could benefit from a .sort method call.

Again, I want to thank you for developing this and getting to MidiTok. I truly think that once achieved (i.e. all MIDI data parsed and written back as original), Symusic + minimidi can durably replace miditoolkit / pretty_midi and maybe even Mido for some usages.
In the case of MidiTok, it would allow to entirely get rid the current json conversion step which is performed before training models, as loading MIDIs on the fly can be a bottleneck.

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Nov 30, 2023

  • For time unit, we have found a way to support MIDI Tick, quarter, second at same time, without introducing additional memory overhead. I'll try to add this new feature. Thanks for you advice.
    #include<iostream>
    #include<cstddef>
    #include<cstdint>
    
    enum class TimeUnit : uint8_t {
        tick,
        quarter,
        second
    };
    
    
    class EventBase {
    protected:
        union {
    	    uint32_t tick;
    	    float quarter;
    	    float second;
        };
        TimeUnit timeUnit;
    };
    
    class ControlChange : public EventBase {
    protected:
        uint8_t number;
        uint8_t value;
    };
  • sort function could be added easily. We just forgot about it before.
  • I did reproduce the errors in failed_midis on MacOS and Windows. However, I can't reproduce it on Linux(just no errors). It's quite a strange issue. I'll keep looking for the cause of the problem.
  • For empty.mid, I didn't get any error iterating over its TrackList(on Linux, MacOS and Windows), like this:
    s = symusic.Score("empty.mid")
    for track in s.tracks:
        for note in track.notes:
            print(note)

@Yikai-Liao
Copy link
Author

@Natooz I have refactored almost all the code using template, which allows users to choose the time_unit by themselves.

The new version of symusic v0.1.0 have been uploaded to pypi, although I haven't update the README file, tutorial.ipynb and symusic.pyi. I fill fix these problems as soon as possible.

However, it might be easy for you to explore the new version:

from symusic import Score
score = Score("path to your midi", time_unit="tick")
for track in score.tracks:
    for note in track.notes:
        cur_tick = note.time  # Here, for all the event, I use time attribute to represent the time stamp (instead of start for Note)
        duration = note.duration

@Natooz
Copy link
Owner

Natooz commented Dec 2, 2023

@Yikai-Liao This is great!
I got some time this morning to test it. I managed to make more comprehensive tests. I updated the gist, you can take a look, almost every MIDI content is tested. This can serve as a reference to continue to fix the bugs until having a version ready to be integrated 🙌

I added "TODOs" notes in the code where I found things that could be improved, namely:

  • adding an end property to the NoteTick;
  • symusic.core.ControlChangeTick object has no attribute number;
  • TrackTick has no program, is_drum and name attributes;
  • .sort method calls for $List classes;
  • Sustain pedals in Track;
  • accepting pathlib.Path (preferred way to deal with paths in Python) objects when loading a MIDI;
  • Unresolved attribute warning for ticks_per_quarter, lyrics and markers from Score;
  • More of a question: I think you left a print("PitchBend") somewhere in the code of Symusic, but I didn't checked;

When I run it on the same MIDI files, I get the same errors for the notes, and one file has errors with tempos, and the last one has a key signature mismatch, its key is -4 whereas the one loaded with MIDIToolkit has key_number=8.

POP909_191.mid: 105 errors (0.05)
POP909_008.mid: 156 errors (0.09)
6338816_Etude No. 4.mid: expected 46 tempos, got 49
6338816_Etude No. 4.mid: 3 errors (0.00)
POP909_022.mid: 13 errors (0.01)
POP909_010.mid: 127 errors (0.07)
6354774_Macabre Waltz.mid: 1 errors (0.00)

I also tested on the multitrack files, and got more errors but I think we can keep on working the "one_track" ones for the moment.

I did not have time to really explore the code of Symusic, I'll try to do so and maybe send PR if I find things where I can help.
And for the two following weeks, I will not be 100% available (conference + vacation) but I'll try my best to answer in the best delay :)

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Dec 2, 2023

Thanks for your reply. We are grateful for your help in testing it!

Maybe there's something wrong with my expression, I mean that even if I tweak some of the bindings, debugging tool might be able to help you to see the new interface.

Also, after the refactoring, the code is a lot more abstract and indeed harder to read.

Btw, I think I should also prepare more for the my 12.10 gre test these days

@Natooz
Copy link
Owner

Natooz commented Dec 6, 2023

@Yikai-Liao @lzqlzzq thank you for the active development of symusic and minimidi!
I run the tests with the new update, on both one track and multitrack MIDI test files, all the content is parsed exactly as it should, congratulations! 🎉
There are still a few tests not passing, but that's only edge cases up to debate, and not problematic in my opinion to begin integrate symusic in miditok.
Here are the test logs, with the explanations of each fail commented:

/Users/nathan/git/MidiTok/venv/bin/python /Users/nathan/git/MidiTok/tests/benchmark_symusic.py 
Loading MIDIs:  59%|█████▉    | 16/27 [00:16<00:09,  1.13it/s]
6354774_Macabre Waltz.mid: 1 errors (0.00)  # key signature number mismatch
Loading MIDIs:  67%|██████▋   | 18/27 [00:17<00:05,  1.68it/s]
Mr. Blue Sky.mid: 1 errors (0.00)  # same as above
Loading MIDIs:  70%|███████   | 19/27 [00:17<00:04,  1.70it/s]
Les Yeux Revolvers.mid: 1 errors (0.00)  # same again
Loading MIDIs:  74%|███████▍  | 20/27 [00:18<00:03,  1.79it/s]
Funkytown.mid: expected 1time signatures, got 3  # 2 last time sigs are likely to be filterd by mido
Funkytown.mid: 2 errors (0.00)
Loading MIDIs:  78%|███████▊  | 21/27 [00:18<00:03,  1.72it/s]
Aicha.mid: expected 17 markers, got 30  # all extras markers feature empty text values
Aicha.mid: 13 errors (0.00)
Loading MIDIs:  96%|█████████▋| 26/27 [00:22<00:00,  1.50it/s]
Girls Just Want to Have Fun.mid: 1 errors (0.00)  # key sig mismatch
Loading MIDIs: 100%|██████████| 27/27 [00:23<00:00,  1.17it/s]
+----------------------------------+
|  MIDI loading speed comparison   |
+-------------+--------------------+
|     Lib     |       Speed        |
+-------------+--------------------+
| MidiToolkit | 0.5160 ± 0.516 sec |
|   SyMusic   | 0.0014 ± 0.001 sec |
+-------------+--------------------+

I updated the test script to handle the symusic update, and left some todo notes for things that could benefit from some improvement (in symusic and/or just the test script itself).
In summary, they are:

  • During the MIDI parsing, Symusic prints "PitchBend" everytime a pitch bend is parsed, I assume as there are the same number of pitch bend than number of messages printed (I removed them from the log above for readability). This is a bit disturbing ;
  • Maybe the .sort method should be made inplace by default, in order to match its common use in Python? What do you think?
  • For the markers error, the extras markers parsed by Symusic all have empty text values (""), which I suppose mido is filtering;
  • We should figure out the key signature number mismatch. I didn't dig into this, maybe everything is already good and the info is just parsed differently in symusic vs miditoolkit;
  • For the time signature error, the two extras parsed by Symusic are TimeSignature(time=79417, numerator=43, denominator=0), TimeSignature(time=79678, numerator=165, denominator=0), which aren't real sigs. Miditoolkit doesn't filter them, so I believe mido must do it (haven't checked);
  • Parse the sustain pedal messages in Track. This is however not a priority and can definitely be done in MidiTok, so if you don't want to do it I'll totally understand;
  • Accept pathlib.Path objects as paths to the MIDI file to load.

For the markers and time signatures, I don't know if we should filter them as mido might do. As the library plays the role of a parser, it is intended to retrieve the data exactly as it is within the file without altering (filtering) it even if it is invalid our outside the recommended values, leaving this last task up to the user. But maybe a nice feature would be to offer users a ‘filter_unrecommended_values‘ argument when loading the MIDI, that would make checks when parsing time signatures, markers, lyrics or other concerned events, and not parse those containing values outside the recommended values in the General MIDI specifications.

In the coming days I'll open a PR here for the integration of Symusic. This will mark either the 2.2.0 or 3.0.0 version, depending on the amount of changes. The plan is to remove miditoolkit for the requirements, replace it with symusic, but still support miditoolkit.MidiFile objects for the tokenizer.midi_to_tokens() method for retrocompatibility by converting it to a symusic.Score object. For the writing process, I'll probably begin by making a method calling mido. Ultimately it would be great for users if symusic could do it. I don't realize the work load for this, do you have any clue/directions?

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Dec 6, 2023

Thanks for your test.

Simple Parsing Error and Design Problems

  • I did forget to delete cout << 'PitchBend'
  • I'm currently focusing more on the accuracy and function support. So interface is not well designed. So I'll follow your advice on sort interface. In addition, should alter other operation like shift_time to inplace operation as default? Well currently, I offer both copy and inplace version for a lot of methods, with out inplace arguments. I formerly decided to align with numpy, using copy as default for them. I wonder what's your advice.
  • ScoreFactory do accept Path as arguments, the problem is that I forgot to add a conversion.
@dataclass(frozen=True)
class ScoreFactory:
    __core_classes = CoreClasses(core.ScoreTick, core.ScoreQuarter, smt.ScoreSecond)
    
    def __call__(self, file: Union[str, Path], ttype: smt.GeneralTimeUnit = TimeUnit.tick):
        if isinstance(file, str) or isinstance(file, Path):
            return self.from_file(file, ttype)
    
    def from_file(self, path: Union[str, Path], ttype: smt.GeneralTimeUnit = TimeUnit.tick) -> smt.Score:
        a = self.__core_classes.dispatch(ttype)
        return a.from_file(path)

Hard Problems

  • I have added pedal events, I'll release it later. Because I found that currently, miditoolkits will get different number of control change events compared to symusic. I want to fix it, but its a bit hard. May be I'll fix it after my gre test(12.10).
  • For text events, symusic works properly on mahler.mid. Well, I only choose to parse lyric and marker events in midi. I wonder if the data you are talking about is stored in other type of messages?
  • For type hints, well, I failed to write things like Note, which can be used as a type hint for NoteTick, NoteQuarter, and NoteSecond. The problem is about python protocol and generic type (At first, I thought it was bug of type checker, so I created an issue. Currently, I just use Union as a type hint. I wonder if there would be a better solution?

Future

  • For writing, actually, we have added some untested methods into minimidi. But my energy is limited, so I tend to prioritize solving parsing errors. Once we add writing methods, we'll update symusic to 0.2.0.
  • Also, for our future map, we are also unsatisfied with existing library (music21) in python with the ability to parse musicxml and abc notation. So I think I'll further support those formats and unify them into Score in my winter vacation.

@Natooz
Copy link
Owner

Natooz commented Dec 6, 2023

  • for the .sort, as it is implemented for classes mimicking the python list, I think it feels natural to have it inplace as users will probably use it. For other methods, I think you did well to use copies and not performing things inplace by default. Memory is very unlikely to be an issue, even when dealing for big MIDI files. Now I think I would have probably aligned with how pandas deals with it, that is only providing one method and an inplace argument instead of one method for each mode.
  • pathlib.Path: fantastic!
  • I get the same control changes for the two libraries, however thanks to you I just noticed that there was an error in miditoolkit for the parsing of pedals: Fixing values for parsing of pedals YatingMusic/miditoolkit#48 With this fix, all the tests for pedals are passing!
  • I don't know, didn't check what Mido is doing yet (maybe I'll do tomorrow), but the extra Markers have an empty text value ("").
  • for the type hints, at least thank you for trying! We can maybe focus on this later, as its only a conveniency for developers and isn't necessary for the final real condition usages;
  • Writing: great! Depending on your progress, I'll maybe wait to release the next major version of MidiTok until this is implemented. That would be preferable, but of course we all have limited time so no pressure, what you already did is great!
  • That would be fantastic if you manage to bring compatibility with these formats! And even more with features from these libraries! I already have no doubt that symusic will bring great benefits to the community!

@Natooz
Copy link
Owner

Natooz commented Dec 9, 2023

I have begun the switch to symidi, it's going well but there are still a few things blocking its full completion:

  • we need a way to create an empty Score without reading any file, such as midi = Score(ticks_per_quarter=480);
  • we need a way to identify the types with isinstance();

Additionally, the PedalTick class yields a "Expected type 'bool', got 'int' instead" warning for the duration argument;

I'll take a look at those if I have time before you do.
Right now, the tokenization step is working, only the detokenization is blocked (first bullet point above). Didn't try the other tests.
I'll soon open the PR, I just wanted to do as much as possible before. The tests will fail first as there are still a lot of things to fix / improve.

@Natooz
Copy link
Owner

Natooz commented Dec 9, 2023

Also it would be great to implement the __eq__ magic methods for the "container" (Note...) and Score classes. They are somehow implemented as they are dataclasses, but only work when comparing the exact same object:

midi = Score(midi_path)
midi2 = Score(midi_path)

midi.tracks[0].notes[0] == midi.tracks[0].notes[0]
# True

midi.tracks[0].notes[0] == midi2.tracks[0].notes[0]
# False

@Yikai-Liao
Copy link
Author

  • I have added a ttype() method for all the objects like ScoreTick, NoteTick, NoteTickList. I think this is good way to identify the time unit.
  • ScoreFactory now have a from_tqp methods, and you could also use Score(480) or Score(x=480, dtype="tick"), the ScoreFactory will dispatch the right method for you based on the type of x
  • Temporarily, I can't release a new version, owing to the compilation error on mac arm platform. I'm waiting for the support from the author or zpp_bits. Here is the issue

@Natooz
Copy link
Owner

Natooz commented Dec 9, 2023

Great, thank you for the very prompt reaction!
No worry for the current compilation issue :)

For the first bullet point, I was referring to the base type of the objects. It seems to need to be addressed when using pybind: https://stackoverflow.com/questions/76239369/in-pybind11-is-it-possible-to-determine-whether-a-pyobject-is-a-pydict

Also for the __eq__ method, you can take a look at how it is done in miditoolkit. Normally the __eq__ of dataclasses will automatically compare the values of their attributes, however here it seems to check if the compared element is the exact same object (id).

@Yikai-Liao
Copy link
Author

Well, I do understand the part about __eq__.
But I still don't get what you do you mean, about isinstance(). Could you give me a few examples about the usage?

@Natooz
Copy link
Owner

Natooz commented Dec 10, 2023

Of course, here is an example in MidiTok, in the __call__ magic method where MidiFile is replaced by Score, that's allows to directly route to the expected method.

@Yikai-Liao
Copy link
Author

Like this?

from symusic import Score
from symusic.core import ScoreTick
s = Score("path")
assert isinstance(s, Score)  # fail, but you want to make is pass
assert isinstance(s, ScoreTick)  # pass

@Natooz
Copy link
Owner

Natooz commented Dec 10, 2023

Yes that's it!
I just assumed that there would be some inheritance here making it work with Score, I'll just use ScoreTick as we are working in ticks anyway.
Apologies for bothering you with this :)

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Dec 10, 2023

Well, it's hard to make isinstance(s, Score) work, because Score is not a class. it is an instance of ScoreFactory.

On the other hand, the purpose of putting all the things from c++ to core is that, I don't want to expose those annoying NoteTick, NoteQuarter, NoteSecond to users. Directly checking ScoreTick may lead to another 2 if-else statement if you want to support other time-unit (although they all will have simple convertion methods among each other)

Maybe there is a better encapsulation method?

What about writing a isinstance(alignwith maybe a better name) method for all those factory class?

@Natooz
Copy link
Owner

Natooz commented Dec 10, 2023

I agree with you that it's nicer to not expose *Tick, *Quarter *Second classes.

Maybe an isinstance for Score covering these subclasses could have some interest/use cases? That's ok for me to not have it.

Now, do you think we could have interfaces to create Note, Track, Tempo etc classes from there attributes, such as note = Note(my_time, dur, pitch, vel)?

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Dec 10, 2023

Doesn't the current version work?

@dataclass(frozen=True) 
 class NoteFactory: 
     __core_classes = CoreClasses(core.NoteTickcore.NoteQuartercore.NoteSecond) 
  
     def __call__( 
         self, 
         timesmt.TimeDtype, 
         durationsmt.TimeDtype, 
         pitchint, 
         velocityint, 
         ttypesmt.GeneralTimeUnit = TimeUnit.tick, 
     ) -> smt.Note: 
         """ 
         Note that `smt.TimeDtype = Union[int, float]`, and Note constructor requires `int` or `float` as time. 
         So Type Checker like MyPy will complain about the type of `time` argument. 
         However, float and int can be converted to each other implicitly. 
         So I just add a `# type: ignore` to ignore the type checking. 
         """ 
         return self.__core_classes.dispatch(ttype)(timedurationpitchvelocity)  # type: ignore

@Natooz
Copy link
Owner

Natooz commented Dec 10, 2023

It's working for notes, tempos etc
Just missing for Track

@Natooz
Copy link
Owner

Natooz commented Dec 11, 2023

Wonderful! 🤩
I'll test it in the coming days!

@Natooz
Copy link
Owner

Natooz commented Dec 15, 2023

I have begun the new tests, right now 50 out of 136 tests on single track tokenization - detokenization are passing! 🙌
A lot of things need to be fix/adapted within MidiTok, which I'm taking care of. I'll keep you updated.

On the meantime, would it be possible to have a .end property for the Pedal class, similarly to Note?

@Yikai-Liao
Copy link
Author

ok,I'll add it

@Natooz
Copy link
Owner

Natooz commented Dec 17, 2023

@Yikai-Liao there is still one test not passing, that might come from symusic's writing - loading (but not sure 100%). I'll try to add tests to the library now in order to make sure.

Also, while when I load back a midi saved with symusic I can retrieve all the tracks with their programs, when I open it with a DAW (Logic Pro), all the tracks have the same program, being the same as the first one, except for drums tracks. There may be a channel mismatch or something else in the writing process?

Here are two examples: Archive.zip
The originals are in the miditok tests repo

@Yikai-Liao
Copy link
Author

@Yikai-Liao there is still one test not passing, that might come from symusic's writing - loading (but not sure 100%). I'll try to add tests to the library now in order to make sure.

Also, while when I load back a midi saved with symusic I can retrieve all the tracks with their programs, when I open it with a DAW (Logic Pro), all the tracks have the same program, being the same as the first one, except for drums tracks. There may be a channel mismatch or something else in the writing process?

Here are two examples: Archive.zip
The originals are in the miditok tests repo

well,in symusic, there is no channel information. there is only program currently. So for convenience, when writing midi back, I just set all track except for drum to channel 0. I'm not sure will this cause a problem.

@Yikai-Liao
Copy link
Author

@Yikai-Liao there is still one test not passing, that might come from symusic's writing - loading (but not sure 100%). I'll try to add tests to the library now in order to make sure.

Also, while when I load back a midi saved with symusic I can retrieve all the tracks with their programs, when I open it with a DAW (Logic Pro), all the tracks have the same program, being the same as the first one, except for drums tracks. There may be a channel mismatch or something else in the writing process?

Here are two examples: Archive.zip The originals are in the miditok tests repo

I dumped those two midi files with symusic, and asked someone else to load them to Logic Pro and FL studio. He said that, they are loaded correctly.

@Natooz
Copy link
Owner

Natooz commented Dec 18, 2023

Here is what it looks like with Logic Pro

Capture d’écran 2023-12-18 à 09 40 25

I haven't had time to figure what's wrong, but I can tell something is wrong (I never encountered this with other libs) as I just tested it again.

@Yikai-Liao
Copy link
Author

cca3162c7140c74fff3bcdcf72e6ac0
We can't reproduce this problem. There is no problem loading midis both in you zip or dumped by myself

@Natooz
Copy link
Owner

Natooz commented Dec 18, 2023

Great that's good news! I prefer something being wrong just on my machine, now time to find what.
Could you share the MIDI you dumped? Was it dumped on macOS (arm/intel) or something else?

FYI I encounter this bug with symusic 0.2.1, macOS 12.7 intel

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Dec 18, 2023

Great that's good news! I prefer something being wrong just on my machine, now time to find what.
Could you share the MIDI you dumped? Was it dumped on macOS (arm/intel) or something else?

FYI I encounter this bug with symusic 0.2.1, macOS 12.7 intel

The midi is exactly the same as yours. And we test it on m1 and m2
mmexport1702891431946.jpg
mmexport1702891958319.png

@Natooz
Copy link
Owner

Natooz commented Dec 18, 2023

I meant that maybe the MIDI might have been dumped differently from the different platforms? Or maybe this might just come from Logic Pro? I can't find where to see the version. I'll be able to test on my second machine in a few hours, which has an other version installed.

@Yikai-Liao
Copy link
Author

Music.zip
well, I just read your midi files and re-dump them (on Linux).
I thought they were same. However, when looking at there md5, they are different.
But both these two version can be loaded correctly.

@Natooz
Copy link
Owner

Natooz commented Dec 18, 2023

I have the same problem with these files, so the issue must come from my installation (Logic 10.7.4). I'll test on my desktop when I'll get home. But I assume there is no issue/bug within symusic then.
Thank you for sharing the files!

@Natooz
Copy link
Owner

Natooz commented Dec 18, 2023

I have the same issue on my other machine, with Logic Pro 10.6.0.
I doubt that the issue comes from my two machines.

@Yikai-Liao
Copy link
Author

I have the same issue on my other machine, with Logic Pro 10.6.0.
I doubt that the issue comes from my two machines.

Maybe you could try fl studio, which also works as excepted in my test.

@Natooz
Copy link
Owner

Natooz commented Dec 19, 2023

It can be opened normally with signal. I don't know what to think about it, I'll just leave it for later maybe and focus on more important matters (tests, integration...)

Copy link

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Inactive since 30 days or more label Jan 10, 2024
@Natooz Natooz removed the stale Inactive since 30 days or more label Jan 10, 2024
@Natooz
Copy link
Owner

Natooz commented Jan 10, 2024

Leaving this open until the v3 is finally released!

@Natooz
Copy link
Owner

Natooz commented Jan 17, 2024

@Yikai-Liao @lzqlzzq 👋

I'm (finally 😅) about to release the V3! I took more time than I expected, but I also changed/improved a lot of things. There are still a few things that would need to be fixed (especially tokenization tests not passing on a few files from the MMD dataset, using bindings for the tokenization part), I leave it for later, the benefits we have right now are already huge!
I'll report here a few benchmarks to be linked in the release description.

Before releasing, do you plan to make updates of symusic / minimidi in the coming days? I'm asking so that I can directly set the minimum symusic version to the very latest.

Also, I'll make some promotion for symusic on twitter. If you have accounts, I can link to them when crediting you.

@Natooz
Copy link
Owner

Natooz commented Jan 17, 2024

CPU: i7-6700K
symusic: 0.3.2
miditoolkit: 1.0.1
pretty_midi: 0.2.10

MIDI file reading:

Maestro MMD POP909
Symusic 1.94 ± 1.87 ms 0.55 ± 0.59 ms 0.26 ± 0.06 ms
MidiToolkit 0.62 ± 0.54 sec 0.20 ± 0.21 sec 0.11 ± 0.03 sec
Pretty MIDI 0.62 ± 0.55 sec 0.20 ± 0.21 sec 0.11 ± 0.03 sec

Symusic is 320 faster for Maestro, 364 for MMD, 423 for POP909.

MIDI preprocessing

#### Maestro Dataset

MidiTok version Maestro - REMI Maestro - TSD Maestro - MIDILike Maestro - Structured
2.1.8 41.62 ± 35.15 ms 40.18 ± 32.54 ms 37.66 ± 29.76 ms 38.46 ± 30.30 ms
3.0.0 0.93 ± 0.49 ms 0.92 ± 0.47 ms 0.75 ± 0.38 ms 0.60 ± 0.42 ms

MetaMIDI Dataset

MidiTok version MMD - REMI MMD - TSD MMD - MIDILike MMD - Structured
2.1.8 22.01 ± 24.27 ms 21.28 ± 23.18 ms 22.39 ± 24.32 ms 19.97 ± 21.18 ms
3.0.0 3.93 ± 10.86 ms 3.92 ± 10.99 ms 3.70 ± 10.88 ms 0.69 ± 0.46 ms

POP909 dataset

MidiTok version POP909 - REMI POP909 - TSD POP909 - MIDILike POP909 - Structured
2.1.8 12.54 ± 4.22 ms 11.59 ± 4.08 ms 12.61 ± 4.78 ms 12.15 ± 3.92 ms
3.0.0 0.52 ± 0.16 ms 0.52 ± 0.17 ms 0.42 ± 0.16 ms 0.25 ± 0.04 ms

Tokenization (MIDI already loaded)

#### Maestro Dataset

MidiTok version Maestro - REMI Maestro - TSD Maestro - MIDILike Maestro - Structured
2.1.8 140.22 ± 109.28 ms 161.75 ± 126.79 ms 146.12 ± 113.57 ms 148.68 ± 113.73 ms
3.0.0 74.96 ± 62.76 ms 109.31 ± 91.38 ms 124.17 ± 99.40 ms 114.77 ± 91.43 ms

MetaMIDI Dataset

MidiTok version MMD - REMI MMD - TSD MMD - MIDILike MMD - Structured
2.1.8 77.22 ± 85.86 ms 89.75 ± 95.08 ms 81.05 ± 85.11 ms 74.09 ± 75.98 ms
3.0.0 43.94 ± 48.88 ms 61.61 ± 66.05 ms 68.01 ± 75.09 ms 59.32 ± 61.83 ms

POP909 dataset

MidiTok version POP909 - REMI POP909 - TSD POP909 - MIDILike POP909 - Structured
2.1.8 39.70 ± 15.04 ms 44.61 ± 16.37 ms 43.90 ± 14.79 ms 41.88 ± 15.29 ms
3.0.0 18.91 ± 11.29 ms 24.96 ± 10.95 ms 31.85 ± 12.95 ms 28.17 ± 12.31 ms

@Yikai-Liao
Copy link
Author

Yikai-Liao commented Jan 17, 2024

Now, the newest version of symusic is 0.3.2, which fixed some bugs we just found. We will try to complete the (now still empty) documentation in a subsequent 0.3.* update.

We do have a more clear plan for the next 2 large versions now. We plans to add support for abc notation and audio synth. And now, we are developing them in parrallel.

We have previously written a command line tool for midi audio synthesis based on soundfont that is not open source. It's not quite finished, but it serves as a good base for symusic's next development goals.

The support for abc notation is more complicated, because writing a syntax parser from scratch is more difficult, the parse part of abc.js uses more than 6000 lines of js code. Perhaps using the existing abc2midi command line tool is a better choice, and it's also fast actually. Anyway, there are still determinations to be made about the exact technical route of abc notation.

BTW, I don't get a twitter account now. Maybe I should sign up for one.

@Natooz
Copy link
Owner

Natooz commented Jan 17, 2024

That's noted, thank you for the info!

Ultimately, a dedicated C parser will always be faster, but I of course acknowledge the difficulty of writing one from scratch.

Tell me if you plan to create an account ;)

@Yikai-Liao
Copy link
Author

My brand new twitter account: RealYikaiLiao

@Natooz
Copy link
Owner

Natooz commented Jan 17, 2024

End-to-end tokenization benchmark (MIDI read + tokenization)

Performed on a set of 50 MIDIs with an intel i7-6700K CPU.

Maestro Dataset

MidiTok version Maestro - REMI Maestro - TSD Maestro - MIDILike Maestro - Structured
2.1.8 947.32 ± 797.81 ms 928.67 ± 792.42 ms 876.67 ± 762.02 ms 882.58 ± 771.79 ms
3.0.0 73.49 ± 59.18 ms 107.65 ± 86.84 ms 124.82 ± 100.16 ms 125.63 ± 105.49 ms

MetaMidi Dataset

MidiTok version MMD - REMI MMD - TSD MMD - MIDILike MMD - Structured
2.1.8 300.40 ± 323.59 ms 315.35 ± 332.46 ms 325.02 ± 367.94 ms 300.58 ± 312.62 ms
3.0.0 41.42 ± 43.03 ms 60.46 ± 61.78 ms 69.38 ± 68.02 ms 65.77 ± 67.71 ms

POP909 Dataset

MidiTok version POP909 - REMI POP909 - TSD POP909 - MIDILike POP909 - Structured
2.1.8 141.10 ± 29.07 ms 147.07 ± 29.89 ms 145.23 ± 32.78 ms 143.50 ± 30.81 ms
3.0.0 18.48 ± 10.30 ms 25.08 ± 10.50 ms 31.75 ± 12.75 ms 28.56 ± 12.08 ms

@Natooz
Copy link
Owner

Natooz commented Jan 19, 2024

Closing this as the V3 is finally released! :)

@Natooz Natooz closed this as completed Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants