-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chroma shifted when using more than 12 bands #391
Comments
Hey, thanks for filing the issue. We'll look into it as soon as we can :) |
Thank you! Happy to help out any way I can, though my knowledge of audio analysis is pretty scant. |
Updating here to say that as far as I can tell this bug still exists and I'm fairly sure the transposition error is happening somewhere in this function, but I lack the theoretical understanding to parse out where Line 191 in 4888cdd
|
Hey @harrislapiroff, sorry it's been over a year. I'm not really sure why this is happening. @odub I wonder if you have any ideas? |
@harrislapiroff thanks for the beautifully documented example. Some cases like that would be valuable in the test suite if you have more appetite to work on this :) I can't pick up the implementation on the fix but here's a driveby for one thing I see going wrong that might help you. This code to make the output data C-based rather than A-based won't work for any number of chroma bands other than 12. Its job is to shift the bottom three bins of the output data to the end of the array assuming that the bins correspond to semitones (3 semitones from A to C). Where that's not the case we're at sea. Scaling that number based on the number of chroma bands might be a way forward towards supporting other numbers of divisions correctly. For the 24-band case, it looks like shifting by 6 rather than by 3 would get you a more expected output. |
I can't guarantee there won't be other issues even where that one is fixed, but that's one I see for now anyway |
Thanks for looking into it @odub! That makes sense. I'll look into patching this. |
Just glancing at this issue again and noticed my example case has slightly different wrong behavior now in my browser. It looks like when extracting to 24 bands middle C is indeed now at the 0th band, but when extracting to 12 bands it's the 10th(?) Was there a deliberate behavior change to chroma extraction in the intervening years, out of curiosity? |
Uh, not intentionally! 😬 I wonder if you know what version you were using originally? |
Hmm looking at it again I'm now seeing the same wrong behavior I expected. Not sure what was going on there. I added a dropdown to my notebook to switch versions though, which should help with any debugging. |
Thanks! |
When using 12 bands for chroma extraction (the default configuration) C is the zeroth band. When using more than 12, C seems to be shifted up to be a higher band.
Steps to reproduce:
Expected result: Value of band 0 should be 1, all other bands < 1
Actual result: Value of band 3 is 1, all other bands < 1
See interactive reproduction here (best in Firefox).
The text was updated successfully, but these errors were encountered: