Skip to content
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

Amend upper frequency limit of mel bands in NoveltySlice to 20kHz #87

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

weefuzzy
Copy link
Member

closes #86

This is a trivial change, fixing what seems to be a typo in the NoveltySliceClient. However, existing example code and stuff you might all be working on that uses novelty slicing with MFCCs will possibly be affected and need re-tuning. Hence a PR rather than just changing it.

The expected results on the test will certainly need to change.

@weefuzzy weefuzzy linked an issue Jan 28, 2022 that may be closed by this pull request
@jamesb93 jamesb93 added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 28, 2022
@jamesb93 jamesb93 self-assigned this Jan 28, 2022
@tedmoore
Copy link
Member

thanks for the heads up!

@tremblap
Copy link
Member

happy to change, although the 5k idea of @g-roma seems to make sense too. we could also change the number of mfccs to 20 instead of 13 if we go higher def?

Copy link
Member

@jamesb93 jamesb93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer 13 MFCCs as the default so think this is ready to merge.

@tremblap
Copy link
Member

tremblap commented Feb 1, 2022

The default is also 5k if you read @g-roma right... if we're breaking this, we could try a few options on ranges. Do we really care about 10k to 20k for instance, or sharp changes. 20 bands is also common for music material, etc

@weefuzzy
Copy link
Member Author

weefuzzy commented Feb 1, 2022

I think it ought to have the same behaviour as our MFCC object does by default, which is 20k. I can imagine 5k being used for speech research where sampling at 11k was SOP for quite some time, and perhaps for things where you can be sure that high frequency energy isn't that structured, but I don't think it's a useful restriction for a change detector where the control is fixed (and will stay fixed, before anyone gets any ideas).

I don't think there's anything to be gained by embarking on a adventure of experiments correcting something that was a mistake to begin with. When we have a separate object for making novelty features, people can freestyle as they please.

@tremblap
Copy link
Member

tremblap commented Feb 1, 2022

ok let's park optimisation indeed (you all seem to agree so why should I party poop :)

@tremblap
Copy link
Member

tremblap commented Feb 2, 2022

@weefuzzy this would be ready to merge in right? When you do so, feel free to make an issue of the UT not working and putting my face on it

@weefuzzy weefuzzy merged commit c59ece5 into flucoma:dev Feb 2, 2022
@weefuzzy weefuzzy deleted the fix/novelty-mfcc-bandwidth branch February 2, 2022 17:05
fearn-e pushed a commit to fearn-e/flucoma-core that referenced this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoveltySlice w/ MFCCs: Mel frequency range correct?
4 participants