Conversation
…es9, now checks all 15 values. Changed TruncatedIQM's tests to stream error messages to the EXPECTs rather than qDebug().
…80 already existing). Kept 6 obsolete entries.
Owen suggested a better description in the manual. I used it now here as well.
build/depends.py
Outdated
There was a problem hiding this comment.
it looks like you added this file but forgot to add it to git. Please add this file and the corresponding header file. Then we can test your changes.
There was a problem hiding this comment.
Oops, sorry. I've just pushed that.
src/movinginterquartilemean.h
Outdated
There was a problem hiding this comment.
Sorry in advance for nit-pick code style comments. :)
Please remove the exclamation points in front of your comments.
…til. Guarded division by 0 in DlgTrackInfo::slotBpmTap. Renamed MovingInterquartileMean::m_List to m_list and m_Queue to m_queue. Removed ! from movinginterquartilemean.h's comments. Minor cosmetic code improvements.
|
This should address this first issues. |
|
By the way, what should I do with files I modified that had an author? Shall I replace the original author with myself in cases where i completely changed the content of the file? |
|
safe to test? |
src/util/movinginterquartilemean.cpp
Outdated
There was a problem hiding this comment.
you can iterate over two variables at the same time. It would be better here to do that instead of the loop body. This makes the code a bit more verbose.
d_sum += *i;
and maybe change i to el or 'it'. Single letter variables usually indicate that they are used as an index. But iterators aren't an index.
|
Sorry for the late note. Github doesn't notify us if new commits are added to a PR we only see new comments. I added a few comments that should help you to make the code more readable. |
|
Issues should be fixed. Is it necessary to use QMap? The insert code is not that complex. |
There was a problem hiding this comment.
This is due to the nature of floating point numbers. They accumulate small round off errors. In this case it should be enough for us if they don't pass with the border you have chosen.
|
did you test how good the results are with a array size of 80? I can see the BPM drifting a lot getting larger and larger (or smaller and smaller). The drift is somewhat stabilized when I reduce the array size (15). |
|
I only made the test you've seen. I've calculated the results manually, but I see Matlab has a truncated mean function implemented (trimmean), so I could generate a couple of tests with a massive amount of numbers. However, if I tap more than 80 beats to a 4/4 song at 125 bpm I get results accurate up to 0.2 bpm. Can you give more details about this drift you saw? |
|
Besides the nitpick comments it looks fine to me. I couldn't reproduce the drift today. I guess my own timing was a bit off yesterday. Thanks for the work |
|
Hi @ferranpujolcamins -- looks like this doesn't merge cleanly (it's a minor fix). Could you update it? |
This fixes bug #1344727: Allow to change the number of taps for BPM averaging Edit. Conflicts: build/depends.py
|
Thanks! Merged. |
6 participants
This fixes bug #1344727: Allow to change the number of taps for BPM averaging Edit.
Now the bpm is calculated via a 80 samples moving interquartile mean which provides an estimation that is robust against outliers, e.g. a missed tap.