craffel / pretty-midi

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

Fix integrated piano roll division by zero #169

Closed cflamant closed 5 years ago

cflamant commented 5 years ago

When supplying a time array to piano roll, if the time array exceeds the length of the "native" piano roll generated based on when the last note finishes, the mean of empty arrays is computed, resulting in warnings and populating the outputted piano roll with 'nan'. Similarly, if the time array has a greater sampling rate than the "native" piano roll, more empty arrays are generated, resulting in 'nan's when numpy.mean is computed.

cflamant commented 5 years ago

To provide a concrete example, this is the test case I implemented:

Play a note at speed 90 starting at 0.01 and ending at 0.05. This generates a "native" piano roll row of [0, 90, 90, 90, 90] at times corresponding to [0., 0.01, 0.02, 0.03, 0.04]

However, if the following time array is passed to get_piano_roll, times = [0., 0.005, 0.01, 0.015, 0.02, 0.025, 0.03, 0.035, 0.04, 0.045, 0.05, 0.055]

the original implementation raises: "RuntimeWarning: Mean of empty slice." "RuntimeWarning: invalid value encountered in double_scalars",

putting 'nan's in the outputted piano roll. The fix I propose remedies this issue so that given 'times' above, the interpolated piano roll is [0, 0, 90, 90, 90, 90, 90, 90, 90, 90, 0, 0], which is what one would intuitively expect from the operation.

My fix should not affect the working cases of the original implementation since it will only intervene when the slice operation would leave an empty list for np.mean().

craffel commented 5 years ago

Looks good, thank you! One small request and then we can merge it in.

cflamant commented 5 years ago

Oops, I forgot about those print statements. I cleaned them up. Thanks!