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

add chroma feature to NoveltySlice #68

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

g-roma
Copy link
Member

@g-roma g-roma commented Nov 26, 2021

This PR adds chroma to NoveltySlice. The new enum is "Spectrum", "MFCC", "Chroma", "Pitch", "Loudness", so code using pitch or loudness would be affected. Tested in SC. The SC class does not need modification, but the helpfiles on all platforms would need to be updated.
For using with tonal material larger kernel sizes than the default (3) are advised.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jamesb93
Copy link
Member

Nice! Would it be possible to maintain as much of the enum as possible so that chroma becomes feature 4?

@jamesb93 jamesb93 changed the base branch from main to dev November 26, 2021 18:38
@jamesb93
Copy link
Member

I also just moved the PR destination to dev as this is where we'll be (hopefully soon) triggering the CI of nightlies.

@g-roma
Copy link
Member Author

g-roma commented Nov 26, 2021

Nice, thanks. Yes it could be moved, although it makes more sense like this to me. I would expect multivariate features like MFCC and Chroma to be easier to work with.

@jamesb93
Copy link
Member

I agree it makes more sense in the long run to have the multivariate descriptors together. I'm for reshuffling the enum and breaking patches in that case as its relatively easy to fix as long as we make users aware.

@jamesb93 jamesb93 added the enhancement New feature or request label Nov 26, 2021
@jamesb93 jamesb93 added this to the beta5 milestone Nov 26, 2021
Copy link
Member

@weefuzzy weefuzzy left a comment

Choose a reason for hiding this comment

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

This is a breaking change for existing CCE code that has been using either Pitch or Loudness as an underlying feature. However, I think sucking that up (but making sure we announce it with big red letters) is better in the longer term than living with an Enum ordering that 'leaks' the order in which things were implemented. (IOW, I agree with Gerard, after some consideration)

This feels like about as much extra weight as can be added to this Client, IMO: the code is unattractively branch-y and magic number-y now, so I'd like to see two things in the future

  1. A refactor of this Client to do the runtime dispatch better than if-else trees and switch cascades.
  2. The planned work on factoring out the novelty function into a lower level object (along with something to do the change point detection) so that folk can use this with their own combinations of features

@jamesb93
Copy link
Member

  1. The planned work on factoring out the novelty function into a lower level object (along with something to do the change point detection) so that folk can use this with their own combinations of features

This should go elsewhere but while we're at the shops, would it look like:

feature buffer -> novelty

So you can calculate the novelty of any nd features?

@weefuzzy
Copy link
Member

This should go elsewhere
yes 😉

but while we're at the shops, would it look like:

feature buffer -> novelty

That's what I imagine

@jamesb93
Copy link
Member

😮

@tremblap
Copy link
Member

I'm ok to make it better and more consistent. but that implies 2 contradicting orders:

  • the one proposed (which could be ok but it is still arbitrary)
  • another one, in order of complexity for the users: energy is simpler. then fft, then we should put mel I reckon, then MFCC, then pitch then chroma

In all cases, I would not change the default kernel size though. We can leave a note in the help.

So this interface question is complicated. If we change the order now, we need to be prepared to change it again when we decide we want to add mel for instance, or any other descriptor. And then what is the rationale of multivariate together is ok but why not at the end?

@jamesb93
Copy link
Member

I'm ok to make it better and more consistent. but that implies 2 contradicting orders:

  • the one proposed (which could be ok but it is still arbitrary)
  • another one, in order of complexity for the users: energy is simpler. then fft, then we should put mel I reckon, then MFCC, then pitch then chroma

In all cases, I would not change the default kernel size though. We can leave a note in the help.

So this interface question is complicated. If we change the order now, we need to be prepared to change it again when we decide we want to add mel for instance, or any other descriptor. And then what is the rationale of multivariate together is ok but why not at the end?

I think keeping it as close to its former self is better than breaking it entirely. Rule of least surprise or something like that. I don't find "energy" (loudness I assume?) to be simpler in the result it produces and in fact I often find it work not that well. I get the most tuneable results with MFCC and Spectrum, so for me having multivariate features that allude to the Foote paper should be preferentially located as the first enums. In that sense the order becomes less arbitrary and more of a conceptual pathway as one might explore the different features. This is of course all moot in the face of breaking the interface apart into many objects anyway potentially.

Copy link
Member

@tremblap tremblap left a comment

Choose a reason for hiding this comment

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

After a quick discussion with @weefuzzy he made me saw the light. Let's get this approved and go forward and ignore my melband addition

@jamesb93 jamesb93 merged commit 0f4a37c into flucoma:dev Dec 8, 2021
@jamesb93
Copy link
Member

jamesb93 commented Dec 8, 2021

With 4 🆗 I merged :) 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants