enforce strictly increasing values in old_time and new_time #145
Conversation
There was a problem hiding this comment.
Thanks a lot for taking this on! Two small changes requested. It also looks like (according to Travis) your code isn't PEP-8 due to some minor nitpicks:
https://travis-ci.org/craffel/pretty-midi/jobs/397626650#L534
Mind fixing those too? Thanks!
pretty_midi/pretty_midi.py
Outdated
| # if they are not monotonic, a warning should be given | ||
| # and they should be made monotonic with np.maximum.accumulate | ||
| def _strictly_increasing(L): | ||
| if all(x<y for x, y in zip(L, L[1:])): |
There was a problem hiding this comment.
I think
if np.all(np.diff(L) > 0)
should be faster, want to check?
There was a problem hiding this comment.
You're right, using diff is twice as fast! I'll replace zip with the diff approach.
There was a problem hiding this comment.
On second thought, applying np.maximum.accumulate will not enforce strictly increasing event times since it can produce repeated event times. Maybe this is still acceptable. If so, we should consider a more relaxed check of if np.all(np.diff(L) >= 0).
There was a problem hiding this comment.
So I believe original_times must be strictly increasing (i.e. no repeats) - it doesn't make sense to map one time to multiple different times. I think it's ok if new_times is monotonic but not strictly increasing, because you can map a span of the original sequence to a single point in the new sequence (basically cropping out part of the sequence).
Based on this I think the correct pattern would be
original_times, unique_idx = np.unique(original_times, return_index=True)
if (unique_idx.size != original_times.size) or unique_idx != np.arange(unique_idx.size):
warnings.warn('original_times must be strictly increasing; automatically enforcing this.')
new_times = new_times[unique_idx]
if not np.all(np.diff(new_times) > 0):
warnings.warn('new_times must be monotonic, automatically enforcing this.')
new_times = np.maximum.accumulate(new_times)Does this make sense? Does it seem right to you?
There was a problem hiding this comment.
Its probably just me, but I don't quite follow. How is original_times being checked for increasingness? Also, I can't think of a case where unique_idx.size != original_times.size -- could you clarify what its checking?
There was a problem hiding this comment.
Okay I have that change in the latest commit. Now I get an error in the travis build during a different part of the call to test_adjust_times:
FAIL: test_pretty_midi.test_adjust_times
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python3.5.5/lib/python3.5/site-packages/nose/case.py", line 198,
in runTest
self.test(*self.arg)
File "/home/travis/build/craffel/pretty-midi/tests/test_pretty_midi.py", line 169, in
test_adjust_times
[n.start for n in pm.instruments[0].notes], expected_starts)
AssertionError
Running this test locally does not reproduce the error for me, so I'm not sure how to proceed.
There was a problem hiding this comment.
This makes sense. That test takes a pretty_midi.PrettyMIDI object which has notes at start times [1, 2, 3, ..., 9] and remaps them as pm.adjust_times([0, 5, 5, 10], [5, 10, 12, 17]). In this new format np.unique([0, 5, 5, 10]) returns indices [0, 1, 3] which correspond to new times[5, 10, 17]. The previous behavior of the code mapped 5 to 12, now it maps 5 to 10. Neither feels obviously correct to me so as long as it's documented I think it's fine. You should be able to fix the test by changing https://github.com/craffel/pretty-midi/blob/master/tests/test_pretty_midi.py#L165 to pm.adjust_times([0, 5, 5, 10], [5, 12, 13, 17]). Does that work?
There was a problem hiding this comment.
I get the same error in travis with your suggested update to the inputs.
I tested the code line-by-line in my ide. (I'm probably doing this testing in an inefficient way but thats off-topic). Here's what I'm seeing in my spyder ide:
With pm.adjust_times([0, 5, 5, 10], [5, 12, 13, 17]) I get
[n.start for n in pm.instruments[0].notes]
Out[194]: [6.4, 7.8, 9.2, 10.6, 12.0, 13.0, 14.0, 15.0, 16.0]
which does not match the test's expectation: expected_starts = [6, 7, 8, 9, 12, 13, 14, 15, 16]
Am I correct that the call is basically
np.interp(range(1, 10), [0, 5, 10], [5, 12, 17])?
In which case maybe we should change expected_starts?
There was a problem hiding this comment.
You're right. One way to do it would be use [7, 12, 13, 17] for new_times and shift expected_starts by 2, I think. Feel free to edit the test to whatever you think makes sense in this case.
There was a problem hiding this comment.
Okay now I'm passing those tests.
I failed tests in the complicated example, so I edited tests there too. I had to understand how np.interp works before I could write out the tests.
I encountered a floating point issue when calculating the onset of the time signature changes. As a workaround, I edited the test to tolerate floating point error.
Anything else you'd like me to take care of for the PR?
pretty_midi/pretty_midi.py
Outdated
| return | ||
| else: | ||
| warnings.warn( | ||
| "Non-monotonicity detected, enforcing strictly" |
There was a problem hiding this comment.
Can you move this if statement/warning out of the function so that the warning can say something like
"original_times does not monotonically increase..."
i.e., specifying which argument doesn't monotonically increase.
There was a problem hiding this comment.
I've moved the warning out of the function. Now there is an if statement with a warning inside of it for each of old_times and new_times.
…d monotonic increase in new_times
tests/test_pretty_midi.py
Outdated
| @@ -213,7 +213,7 @@ def simple(): | |||
| # Original tempo change times: [0, 6, 8.1, 8.3, 9.3] | |||
| # Plus tempo changes at each of new_times which are not collapsed | |||
| # Plus tempo change at 0s by default | |||
| expected_times = [0., 5., 6., 8.5, | |||
| expected_times = [0., 5., 6, 8.5, | |||
There was a problem hiding this comment.
Nitpick: Can you retain the decimal in this 6?
|
Haha yeah it got a little tricky. Thanks for reviewing my PR and for your patience. It was a great learning experience! |
Hello, I've attempted to address issue #126 with this commit. There's a private method _strictly_increasing() that takes a 1d array as input. If the values in the array strictly increase with index, then nothing happens. Otherwise, the function returns the result of running np.maximum.accumulate on the input.