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

User-defined file loading #201

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

User-defined file loading #201

wants to merge 1 commit into from

Conversation

idoby
Copy link

@idoby idoby commented May 3, 2021

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

@a-pillay
Copy link
Contributor

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
Copy link
Owner

craffel commented Feb 14, 2024

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
Copy link
Author

idoby commented Feb 14, 2024

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
Copy link
Owner

craffel commented Feb 14, 2024

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
Copy link
Owner

craffel commented Feb 14, 2024

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

@a-pillay
Copy link
Contributor

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
Copy link
Owner

craffel commented Feb 14, 2024

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
Copy link
Contributor

I have created #241 to implement this feature.

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.

None yet

3 participants