pretty-midi icon indicating copy to clipboard operation
pretty-midi copied to clipboard

User-defined file loading

Open idoby opened this issue 4 years ago • 8 comments

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

idoby avatar May 03 '21 13:05 idoby

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.

a-pillay avatar Feb 14 '24 04:02 a-pillay

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.

craffel avatar Feb 14 '24 15:02 craffel

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...

idoby avatar Feb 14 '24 15:02 idoby

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 avatar Feb 14 '24 16:02 craffel

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

craffel avatar Feb 14 '24 16:02 craffel

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?

a-pillay avatar Feb 14 '24 16:02 a-pillay

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

craffel avatar Feb 14 '24 16:02 craffel

I have created #241 to implement this feature.

a-pillay avatar Feb 14 '24 23:02 a-pillay