# Creating lists of events (pitches, velocities, delta-times, durations) from a MIDI track

I am trying to understand good design patterns in Python and I cannot think of a way to break this huge function into smaller parts without making the code cluttered, overly complex or plain ugly.

I didn't want to clutter my question by posting the whole file. This function itself is already very large, but the class has only two methods: parse_midi() and generate_midi(file_name, file_length).

pitches, velocities, deltas, durations, and intervals are all MarkovChain objects. MarkovChain is a simple class with methods: add_event(event), generate_markov_dictionary(), and get_next_event(previous_event). MarkovChain.src_events is a list of events to generate the Markov chain from. It is a simple implementation of first order Markov chains.

def parse_midi(self):
# on_notes dictionary holds note_on events until corresponding note_of event is encountered
on_notes = {}
time = 0
previous_pitch = -1
tempos = []
delta = 0
for message in self.track_in:
time += message.time
delta += message.time
# There are also MetaMessages in a midi file, such as comments, track names, etc.
# We just ignore them
if isinstance(message, mido.Message) and message.type in ["note_on", "note_off"]:
# some midi files use note_on events with 0 velocity instead of note_oof events
# so we check if velocity > 0
if message.velocity > 0 and message.type == "note_on":
on_notes[message.note] = time
delta = 0
if previous_pitch == -1:
else:
else:
# KeyError means note_off came without a prior associated note_on event!"
# Just ignore them
with ignored(KeyError):
del on_notes[message.note]

previous_pitch = message.note
# Tempo might be many tempo changes in a midi file, so we store them all to later calculate an average tempo
elif message.type == "set_tempo":
tempos.append(message.tempo)
elif message.type == "time_signature":
self.time_signature = self.TimeSignature(message.numerator, message.denominator,
message.clocks_per_click, message.notated_32nd_notes_per_beat)
# some tracks might be aempty in a midi file. For example they might contain comments as track name, and no note events
if len(self.pitches.src_events) == 0:
print("There are no note events in track {}!\n"
"The file has {} tracks. Please try another one.".format(self.selected_track, self.num_tracks))
exit(1)
# a midi file might not contain tempo information at all. if it does, we calculate the average
# else we just assign a default tempo of 120 bpm
try:
self.average_tempo = int(sum(tempos) / len(tempos))
except ZeroDivisionError:
self.average_tempo = mido.bpm2tempo(120)


I'm updating based on @veedrac's opinion and the doc.

You can split the method based on message.type. The logic behind message.type 'note_on' and 'note_off' is little unclear. So I handle the two in the same method __note_on_off.

To make the methods immune to inheritance override, I prepend double '_'.

Each method takes message object and a dict from parse_midias parameters.
__note_on_off is called only when message.type is either 'note_on' or 'note_off', so we can eliminate the check in if isinstance.

Because some of the splitted methods modify parse_midi's local objects those methods have to be defined inside parse_midi and make those names nonlocal or a dict from parse_midi has to be passed as argument to those methods. I pass a dict to maintain state.

def __note_on_off(self,message,d):
d['time'] += message.time
d['delta'] += message.time
# There are also MetaMessages in a midi file, such as comments, track names, etc.
# We just ignore them
if isinstance(message, mido.Message):
# some midi files use note_on events with 0 velocity instead of note_oof events
# so we check if velocity > 0
if message.velocity > 0 and message.type == "note_on":
d['on_notes'][message.note] = d['time']
d['delta'] = 0
if d['previous_pitch'] == -1:
else:
else:
# KeyError means note_off came without a prior associated note_on event!"
# Just ignore them
with ignored(KeyError):
del d['on_notes'][message.note]

d['previous_pitch'] = message.note
# Tempo might be many tempo changes in a midi file, so we store them all to later calculate an average tempo

def __time_sign(self,message,d):
self.time_signature = self.TimeSignature(message.numerator, message.denominator, message.clocks_per_click, message.notated_32nd_notes_per_beat)

def __set_tempo(self,message,d):
d['tempos'].append(message.tempo)


Then I build dispatch dict whose keys are message.type and values are the splitted methods.

I assume mido object is accessible to parse_midi and __note_on_off as I could not trace its root.

def parse_midi(self):
ld = dict(on_notes = {},
time = 0,
previous_pitch = -1,
delta = 0,
tempos = []
)

dispatch = dict(
set_tempo=self.__set_tempo,
time_signature=self.__time_sign,
note_on=self.__note_on_off,
note_off=self.__note_on_off
)

for message in self.track_in:
try:
dispatch[message.type](message,ld)
except KeyError:
print('Unkdown message type {0}'.format(message.type))
sys.exit(1)

# some tracks might be aempty in a midi file. For example they might contain comments as track name, and no note events
if len(self.pitches.src_events) == 0:
print("There are no note events in track {}!\n"
"The file has {} tracks. Please try another one.".format(self.selected_track, self.num_tracks))
exit(1)
# a midi file might not contain tempo information at all. if it does, we calculate the average
# else we just assign a default tempo of 120 bpm
try:
self.average_tempo = int(sum(ld['tempos']) / len(ld['tempos']))
except ZeroDivisionError:
self.average_tempo = mido.bpm2tempo(120)

• aren't the values of on_notes = {} time = 0 previous_pitch = -1 delta = 0 going to be reset each time the for loop calls __note_on_offmethod? note_on with velocity=0 and a note_off event are both considered note_off events. And on_notes dictionary has to be persistent, because it keeps track of playing notes, whose note_off events may be several messages later, and during that time several notes may become on. If anything is not clear you can ask me other questions, or I can edit the original question to explain the situation further. – kureta Mar 30 '15 at 14:26
• @kureta corrected to maintain state in __note_on_off. – Nizam Mohamed Mar 30 '15 at 16:17
• thank you @nizam. I cannot upvote you since my rep is < 15 – kureta Mar 30 '15 at 16:27
• I didn't test the code because you know I can't. Give us feedback if it's working. If works accept the answer. – Nizam Mohamed Mar 30 '15 at 16:56
• Is there a reason you're using name mangling (__x) rather than just normal private methods (_x)? Also, I really don't like the idea of passing raw locals() to the function. There's no reason not to pass parameters properly and it's not even guaranteed to work. – Veedrac Mar 30 '15 at 19:18

Your notion of average tempo makes little sense. A track with four minutes of ♩=120 followed by a 12-second coda of ♩=60 would have a reported average tempo of 90. A more reasonable average would be to divide the total number of beats by the elapsed time.

$$\frac{120 \frac{\textrm{beats}}{\textrm{min}} × 4\ \textrm{min} + 60 \frac{\textrm{beats}}{\textrm{min}} × 0.2\ \textrm{min}} {4.2\ \textrm{min}} \approx 117 \frac{\textrm{beats}}{\textrm{min}}$$

• Thanks, you are absolutely right. I already modify the tempo of resulting midi in a DAW afterwards, for now. But trying to find the reason of odd rhythms would have been a real headache after the application has become more functional. You saved me a lot of time there. In fact now I think that it would be a good idea to give weights to different tempos (tempi?) according to their duration and change the tempo of the resulting midi file probabilistically. Thanks again. – kureta Mar 30 '15 at 21:22