-
Notifications
You must be signed in to change notification settings - Fork 43
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
OOP implementation #138
Open
jmarcosfer
wants to merge
18
commits into
dev
Choose a base branch
from
oo-implementation
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
OOP implementation #138
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lgorithms including vector_string typed params
…TS api, extend manual test
…Factory directly, not instance, use configure on configure method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #64
Main goal
Modified
code_generator.py
, as well as corresponding parts of.cog
files, to produce a OOP implementation of the library, where each algorithm is a class with aconfigure
,compute
, anddelete
method (this last one is provided by Embind and needed to destroy the underlying C++ object from the JS side).Open design questions
configure
-less? I guess internally it resets the algorithm state, but it's strange from a JS user perspective: maybe just exposing areset
method instead?algorithmInstance()
instead ofalgorithmInstance.compute()
) by implementing a baseEssentiaAlgorithm
TS class which extends the JSFunction
class, as explained here.Issues
Separate create and configure methods: failed for 3 algorithms
In the OOP implementation, algorithm
create
andconfigure
are separateHowever this approach failed to compile for algorithms
["MultiPitchMelodia", "PitchMelodia", "PredominantPitch"]
with the following error, as if too many parameters were passed to theconfigure
call. Perhaps related to this upstream issue, maybe @dbogdanov will know what's happening.Modular structure and imports
As a side-effect, we're forced to re-structure the library's main modules (both WASM and TS interface), since now algorithms are no longer methods of a parent
EssentiaJS
class, but each is a class at root module-level.Limitations:
On the TypeScript API, we're forced to find some other way to get the
EssentiaWASM
instance (which used to be passed to the TS constructor:new Essentia(EssentiaWASM)
. For now I've opted for a top-level util function calledready
which expects to be passed theEssentiaWASM
instance and is also in charge of callingessentia::init()
.Opportunities:
We could re-think how the library gets imported, which has usually been confusing and tedious to get right, depending on the platform you're in. Ideally importing the library would:
EssentiaWASM
backend instanceThis could arguably be on it's own PR, but I think it makes sense to address it here since we're already forced to re-structure the module altogether.
Tests
Took the time to write a first test (manual for now) for this new interface, using the
FrequencyBands
algorithm as test case. This can be used in the future to design test structures and edge-cases for #66.Nice-to-haves
Automatic memory management from the JS side
Something equivalent to TensorFlow.js'
tidy
function would be nice to keep track of Essentia algorithm instances in use inside of the function's scope and automatically delete them as soon as thattidy
-like function returns. Not sure how hard this would be to implement. See here and their source code for more info on how TensorFlow.js manage memory.