Yikai-Liao / symusic

A cross platform note level midi decoding library with lightening speed, based on minimidi.
https://yikai-liao.github.io/symusic/
MIT License
108 stars 8 forks source link

Adding tests for `adjust_time` #30

Closed Natooz closed 4 months ago

Natooz commented 4 months ago

WIP

Adds tests for adjust_time (#28)

Failing right now, I'm just opening the PR as a draft Either I don't understand exactly how the method should used, or the method fails with these simples cases. Am I correct using as it is, with the times provided in ticks? When using original_times=[0, 4] and new_times=[0, 2] only the first note is kept. I thought that only the time portion provided were shrunk, the rest being "untouched", but actually the method will only keep the provided time range. Should we also consider uncovered ranges? When using original_times=[0, 4, 8] and new_times=[0, 2, 6] the second and third notes have negative durations (-2 and -6 respectively).

Yikai-Liao commented 4 months ago

Well, I just followed what pretty_midi did. You can find the source code here. And I don't quite understand why.

        # Only include notes within start/end time of the provided times
        for instrument in self.instruments:
            instrument.notes = [copy.deepcopy(note)
                                for note in instrument.notes
                                if note.start >= original_times[0] and
                                note.end <= original_times[-1]]
Yikai-Liao commented 4 months ago

When using original_times=[0, 4, 8] and new_times=[0, 2, 6] the second and third notes have negative durations (-2 and -6 respectively).

For this, I do find something wrong. I wrote a wrong boundary check. The ends of the last notes in the test are 10 and 22, which are out the range. So they shouldn't be include in the answer according to pretty midi. I have fixed it, although I don't the find the negative durations you mentioned

Natooz commented 4 months ago

I didn't create extended tests (edge cases), maybe this could be further improved in the future

Yikai-Liao commented 4 months ago

That's good enough. Thanks.