search

craffel / pretty-midi

Utility functions for handling MIDI data in a nice/intuitive way.
MIT License
856 stars 151 forks source link

User-defined file loading #201

Open idoby opened 3 years ago

idoby commented 3 years ago

Allow passing in a loaded mido object to allow more fine-grained control over how mido loads the MIDI file.

a-pillay commented 7 months ago

I support this PR (assuming conflicts can be resolved in a straightforward way)! This is an extremely convenient way to implement stuff like changing the midi file tempo which appears to be quite complicated to do within the pretty-midi domain.

craffel commented 7 months ago

Not sure how I missed this but thanks for flagging it @a-pillay . My only suggestion is just to provide a new argument rather than overloading midi_file since this certainly isn't a "midi file" when it's a mido object.

idoby commented 7 months ago

Then you run into the trouble of having to check that both arguments weren't passed in. I think this kind of interface is less error-prone for the user as well. Either way, I'm not sure I have the time to fix this...

craffel commented 7 months ago

I wouldn't call that trouble - it's a single if statement, and adding a single if statement is better than making an argument's meaning ambiguous, because the arguments are user-facing.

craffel commented 7 months ago

@a-pillay feel free to pick up this PR if you have time.

a-pillay commented 7 months ago

Sure, I can take a look at it.

To summarize, this change would involve:

  1. Creating a new argument mido_object in pretty_midi.PrettyMIDI with default=None
  2. Handling passed in mido.MidiFile via a new if-else block:
  3. This block would be the first one for priority (alternately should we flag situations when both mido_object and midi_file arguments are populated?)

Could you please confirm this logic before we implement the same?

craffel commented 7 months ago

Yes, that would be fine. I don't think the order of the blocks is terribly important as long as all cases are handled.

a-pillay commented 7 months ago

I have created #241 to implement this feature.