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

MIDI-129: Try to remove PrettyMIDI dependency #5

Merged
merged 12 commits into from
Nov 27, 2023

Conversation

SamuelJanas
Copy link
Contributor

@SamuelJanas SamuelJanas commented Nov 22, 2023

MIDI-129

Getting rid of the dependency on PrettyMIDI so that we could have a little more freedom when it comes to future development.

@roszcz
Copy link
Member

roszcz commented Nov 23, 2023

Looks good! Can you add some tests for this? :)

@SamuelJanas
Copy link
Contributor Author

I've written some baseline tests to see if everything is of the correct type/exists.

The code is now a bit of a mess, I'll move classes that work as containers to a separate file. The new change also made it possible to cut some lines of code in different places. Some additional changes need to be made. to_midi() will probably work with MidiFile now. I'll have to add some changes so that rendering would work without PrettyMIDI.

I'm trying to figure out what's the minimal amount of code I can copy from PrettyMIDI for our functionality to still work properly - without the dependency. We'll need to credit them in the docs for sure though!

@SamuelJanas
Copy link
Contributor Author

    def to_midi_midi_file(self, instrument_name: str = "Acoustic Grand Piano"):
        track = MidiFile()
        program = pretty_midi.instrument_name_to_program(instrument_name)
        instrument = midi_containers.Instrument(program=program, name=instrument_name)

        note_data = self.df[["velocity", "pitch", "start", "end"]].to_records(index=False)

        for velocity, pitch, start, end in note_data:
            note = midi_containers.Note(velocity=int(velocity), pitch=int(pitch), start=start, end=end)
            instrument.notes.append(note)

        track.instruments.append(instrument)

        return track

instrument_name_to_program is I think one of the last dependencies, it's a matter of creating a list that matches general MIDI convention We probably want to keep this functionality as it allows for creating Cello mp3 as simply as specifying instrument_name = "cello".
I've tested loading the file in -> extracting MidiPiece from it -> calling the new to_midi -> loading it in -> exporting to wav
And it works as expected. There's still some experimenting to do.

@roszcz
Copy link
Member

roszcz commented Nov 24, 2023

    def to_midi_midi_file(self, instrument_name: str = "Acoustic Grand Piano"):
        track = MidiFile()
        program = pretty_midi.instrument_name_to_program(instrument_name)
        instrument = midi_containers.Instrument(program=program, name=instrument_name)

        note_data = self.df[["velocity", "pitch", "start", "end"]].to_records(index=False)

        for velocity, pitch, start, end in note_data:
            note = midi_containers.Note(velocity=int(velocity), pitch=int(pitch), start=start, end=end)
            instrument.notes.append(note)

        track.instruments.append(instrument)

        return track

instrument_name_to_program is I think one of the last dependencies, it's a matter of creating a list that matches general MIDI convention We probably want to keep this functionality as it allows for creating Cello mp3 as simply as specifying instrument_name = "cello". I've tested loading the file in -> extracting MidiPiece from it -> calling the new to_midi -> loading it in -> exporting to wav And it works as expected. There's still some experimenting to do.

Maybe we could ignore the instrument/program problem, and just assume everything is a piano? As far as I know modern Digital Audio Workstation software don't really use this property, and manage different instruments independently from the MIDI file.

@SamuelJanas SamuelJanas marked this pull request as ready for review November 27, 2023 12:21
@SamuelJanas SamuelJanas merged commit 7957a85 into develop Nov 27, 2023
3 checks passed
@SamuelJanas SamuelJanas deleted the MIDI-129/load-midi branch November 27, 2023 13:38
roszcz pushed a commit that referenced this pull request Nov 29, 2023
* initial try

* midi for testing

* container classes from prettyMIDI

* most of new init

* migrated functions

* working midi loading

* remove _midi from MidiFile

* change max_ticks

* Further MidiFile adaptation

* move containers and helpers elsewhere

* segmentize init, write to midi

* Dependency removed(?)
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 this pull request may close these issues.

2 participants