Interchanged pre and pos prefixes in peak_detection function #53
Conversation
There was a problem hiding this comment.
It's intriguing.
The code in librosa does exactly the same:
https://librosa.org/doc/latest/_modules/librosa/util/utils.html#peak_pick
As well as the code in madmom:
https://github.com/CPJKU/madmom/blob/main/madmom/features/onsets.py
Are they wrong? What do you think?
|
The result of both operations is then used as the origin parameter when calling sp.ndimage.filters.maximum_filter. According to the documentation: I had read this before the PR but the direction of the shift had confused me. Reading this again (and drawing some examples for different windows) I now think it was okay before the change, and librosa and madmom are right. What made you think they were interchanged? Maybe I'm missing something. |
|
Look at the plot of the moving average and moving maximum thresholds in the onset detection demo notebook. They clearly extend longer to the future despite the fact that pre is greater than pos. Why is this happening? I thought the parameters were interchanged, but it is not clear to me now. |
|
Yep, the plot indeed shows what you said, and is fixed if we interchange the prefixes. It's weird. Maybe we can talk it through in our next meeting, to make sure we're not missing anything. |
The prefixes were mixed up, as described in the linked issue. Now the filters are centered in the correct sample.