craffel/pretty-midi

Piano Roll for drum tracks outputs zeros

ethman opened this issue · 4 comments

If a PrettyMidi object has an instrument with is_drum=True, the piano roll will be empty for that instrument.

Here is a minimal example:

>>> import numpy as np
>>> import pretty_midi
>>> path = 'Track00001/S01.mid'
>>> pm = pretty_midi.PrettyMIDI(path)
>>> pm.instruments[0]
Instrument(program=0, is_drum=True, name="Um sonhador-Leandro e Leonardo")
>>> len(pm.instruments[0].notes)
1076
>>> max(n.velocity for n in pm.instruments[0].notes)
127
>>> pr = pm.get_piano_roll()
>>> np.max(pr)
0.0

The work around is flipping is_drum=False and the piano roll works as expected:

>>> pm.instruments[0].is_drum = False
>>> pr = pm.get_piano_roll()
>>> np.max(pr)
254.0

(Aside: I'm not sure why the max of the piano roll is 254.0, when the max velocity in the note array is 127)

Here's another example, in case that MIDI file is a fluke:

>>> path = 'Track00002/S09.mid'
>>> pm = pretty_midi.PrettyMIDI(path)
>>> pm.instruments[0]
Instrument(program=0, is_drum=True, name="DRUMS")
>>> len(pm.instruments[0].notes)
2244
>>> max(n.velocity for n in pm.instruments[0].notes)
112
>>> pr = pm.get_piano_roll()
>>> np.max(pr)
0.0
>>> pm.instruments[0].is_drum = False
>>> pr = pm.get_piano_roll()
>>> np.max(pr)
112.0

The one thing these two MIDI files have in common is that they only have one instrument (drums) in them. Here's another example with more than one instrument:

>>> path = 'Track00004/all_src.mid'
>>> pm = pretty_midi.PrettyMIDI(path)
>>> [f'{idx}: {inst.is_drum}' for idx, inst in enumerate(pm.instruments)]
['0: False', '1: False', '2: False', '3: False', '4: False', '5: False', '6: False', '7: False', '8: True']
>>> len(pm.instruments[8].notes)
1909
>>> max(n.velocity for n in pm.instruments[8].notes)
120
>>> pr = pm.instruments[8].get_piano_roll()
>>> np.max(pr)
0.0
>>> pm.instruments[8].is_drum = False
>>> pr = pm.instruments[8].get_piano_roll()
>>> np.max(pr)
120.0

I'll also note that I've tried these with and without altering the values for fs and times with the same results.

I've included all three of these MIDI files here.

Looks like this was a deliberate design decision, as per this comment:

# Drum tracks don't have pitch, so return a matrix of zeros
I can't say I understand this as most DAWs that support piano rolls display drums tracks as piano rolls and the official MIDI spec has mappings from MIDI note values to drum notes.

Hi, this is intended behavior: https://github.com/craffel/pretty-midi/blob/master/pretty_midi/instrument.py#L111
Displaying a piano roll for a drum track is not meaningful; I doubt anyone could look at an event happening at pitch 81 and think to themselves "Oh, an open triangle!" If you have an idea for visualizing a drum track as a 2D matrix, I'd be happy to have a PR.

I can understand that logic as well. It's a bit of a corner case. I'm happy with this work around for my use case, and at least this is documented here for the next person who needs a drum "piano" roll.

Watch out when using the piano roll with drum by just flipping is_drum, because it doesn't work if the int(note.end) - int(note.start) == 0 !

I think explicitly setting the "duration" of the drum note makes sense. Btw I also agree with @ethman on the need for "piano roll" for drums :)

piano_roll = np.zeros((128, int(fs * end_time)))
for note in mid.notes:
    note_start = int(note.start * fs)
    piano_roll[note.pitch, note_start : note_start+1] += note.velocity