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 ampcurve, onsetcurve, noveltycurve + buf versions #151

Merged
merged 35 commits into from
Mar 29, 2022

Conversation

weefuzzy
Copy link
Member

@weefuzzy weefuzzy commented Mar 9, 2022

See flucoma/flucoma-core#114

Max objects that produce the features used for slice detection in ampslice, noveltyslice, onsetslice, called fluid.[whatever]curve~ (which we can think about)

@weefuzzy weefuzzy added the enhancement New feature or request label Mar 9, 2022
@weefuzzy weefuzzy requested review from tremblap and jamesb93 March 9, 2022 17:24
@jamesb93
Copy link
Member

jamesb93 commented Mar 9, 2022

I made a reply that might've been more useful here:

flucoma/flucoma-core#114 (comment)

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.

Approved wholeheartedly but we IMO docs should appear in the same PR.

I offered to make some help files for them and add rst files.

@tremblap
Copy link
Member

I love those. as for naming, since they are in effect time series like all other descriptors, I would remove curve, which creates some frictions:

fluid(buf)novelty is a no brainer

fluid(buf)amp : a bit harder and in conflict with fluid(buf)loudness - which is absolute - so we have a sort of detrender: fluid(buf)detrend ? fluid(buf)ampdiff ?

fluid(buf)onset is even more complicated : we do differential but fluid(buf)spectraldiff is awful

let's riff on names. the shared interface is magic and helps to explain so much!

@tremblap tremblap requested a review from tedmoore March 22, 2022 14:35
@jamesb93
Copy link
Member

jamesb93 commented Mar 22, 2022

I love those. as for naming, since they are in effect time series like all other descriptors, I would remove curve, which creates some frictions:

fluid(buf)novelty is a no brainer

fluid(buf)amp : a bit harder and in conflict with fluid(buf)loudness - which is absolute - so we have a sort of detrender: fluid(buf)detrend ? fluid(buf)ampdiff ?

fluid(buf)onset is even more complicated : we do differential but fluid(buf)spectraldiff is awful

let's riff on names. the shared interface is magic and helps to explain so much!

Another angle to approach this from is that the naming interface should explicitly match that of the corresponding slicer.

fluid.bufnoveltyslice~ -> fluid.bufnoveltycurve~
fluid.bufampgate~ -> fluid.bufampcurve~
fluid.bufampslice~ -> fluid.bufampcurve~
fluid.bufonsetslice -> fluid.bufonsetcurve~

While these objects can be features in isolation, creating a strong associative naming between the slicer, and the object that slices on that curve is more meaningful. On the flip, I think something like bufnovelty is much terser and direct, but the convention breaks down, as you pointed out @tremblap for the other objects and that might cause confusion IMO.

I'm not entirely sold on curve~ but haven't got a better suggestion, apart from maybe contour~ which is too long. After all it is a "curve" so maybe not worth reinventing the wheel.

@tremblap
Copy link
Member

the problem I see with adding anything after the feature is that we load it with meaning - you know are careful we've been in trying to keep them as agnostic as possible. I'd like to keep our pattern (also we are not providing curves in realtime but streams)

transient -> transientslice
novelty -> noveltyslice

ampgate -> we don't have anything but loudness @filter 0 is the nearest of an energy curve/stream people get

now let's brainstorm for the problematic 2:

ampslice -> ampdiff ? ampchange ? ampdetrend?

onsetslice -> spectralchange (which is what it is trying to do as a stream/curve with 10 different metrics) ? spectraldiff? spectralflux (although that has already a meaning in descriptor literature)

@tremblap
Copy link
Member

I'll move discussion to the core PR

@jamesb93 jamesb93 added this to the beta6 milestone Mar 28, 2022
@jamesb93
Copy link
Member

Helpfile documentation is done for this now.

@jamesb93 jamesb93 merged commit 0bb6bb4 into flucoma:dev Mar 29, 2022
@weefuzzy weefuzzy deleted the enhance/change-detection-features branch February 20, 2023 09:40
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.

4 participants