search

urinieto / msaf

Music Structure Analysis Framework
MIT License
478 stars 79 forks source link

Allow custom features #139

Closed carlthome closed 9 months ago

carlthome commented 10 months ago

Trying to implement a new feature by implementing Features but unable to use it with msaf.run.process because there's a hardcoded list of feature names in _preprocess in the SegmenterInterface that's not exposed to the user (as far as I can see).

Maybe I'm missing something in the docs but think this check could simply be removed, and the try/except covers any potential problems on its own.

Bit unsure of this though. Gladly take help!

carlthome commented 10 months ago

Additionally, I was unable to create a subclass of msaf.features.Features (trying EnCodec embeddings) to register in collection mode due to how joblib results in cleared out msaf.features_registry dicts per worker (bit of a rabbit hole this: https://joblib.readthedocs.io/en/latest/parallel.html#serialization-processes).

Added a fix on this PR.

urinieto commented 10 months ago

Looks good to me. Could you make sure that the documentation is up to date for this to work? Here's the part of the documentation we should make sure it still works: https://pythonhosted.org/msaf/features.html#adding-new-features-to-msaf

After this is checked, I'll merge the PR.

urinieto commented 9 months ago

Seems like tests are not passing. Will merge after this is fixed. Thanks @carlthome

carlthome commented 9 months ago

Fix for CI failures in https://github.com/urinieto/msaf/pull/143

carlthome commented 9 months ago

Will merge after this is fixed.

Would also like to demonstrate that this actually works first, perhaps by adding a simple example to examples/

urinieto commented 9 months ago

Thanks! Seems like tests are still not passing, will wait before merging.

carlthome commented 9 months ago

Ready to go in now. Manually tested to work in both single file mode and collection mode.

carlthome commented 9 months ago

Although maybe https://github.com/urinieto/msaf/pull/143 should go in first to get down the size of this diff.