Adding a new MSA algorithm in MSAF about msaf HOT 11 CLOSED

I would like to add a MSA algorithm in your MSAF toolbox [1].

Wonderful initiative @ax-le! 👏 Think it's absolutely worthwhile to collect algorithms, and collaborate on making sure everything stays comparable. MSAF feels like a good platform for this.

do you think we should update the way to include new algorithms, or the legacy way should still be functional?

The documented way should be functional. No breaking changes have been introduced, I think, but if I'm mistaken I'll gladly pitch in and resolve any accidental issues!

In particular, would you allow/advise me to implement the easy encaspulation way?

Sounds good to me! Simply including as_seg in install_requires and calling out to your existing code via a SegmenterInterface implementation would be my first attempt. Porting code is always scary, and nice to avoid.

I imagine the version pinning will become immediately problematic however so those would preferably be changed to lower bounds instead. Good news is the dependencies themselves are almost the same as what's already in MSAF (especially the heavy ones like scikit-learn and SciPy), and also including madmom and mirdata sounds good to me. It's of course nice to always try to get rid of unneeded dependencies when possible however (fewer future incompatibility issues to chase down).

from msaf.

Ok perfect, nice to see that the installation went through! :)

I will try a first draft in the next days, and I will comment here again when the PR is available! Thanks for the support :)

from msaf.

Hi Alex, thanks for your interest in adding a new algorithm in MSAF! MSAF is going through some updates as of late, mostly thanks to @carlthome. Carl, do you think we should update the way to include new algorithms, or the legacy way should still be functional?

For reference, here's the documentation on how to add algorithms to MSAF: https://pythonhosted.org/msaf/algorithms.html#adding-a-new-algorithm-to-msaf

from msaf.

Can see how far the installation gets in #147 🤞

from msaf.

Currently stuck on https://github.com/urinieto/msaf/actions/runs/6870581237/job/18685765386#step:5:122

The as-seg release at PyPI specifies sklearn and not scikit-learn. Noticed you've fixed that on https://gitlab.inria.fr/amarmore/autosimilarity_segmentation/-/commit/5ac886b25f716d05d2537f366e5194eb0691f858 but would be good to release that deprecation fix to PyPI too @ax-le!

from msaf.

This is awesome, thanks @carlthome for taking the initiative on this. Seems like once as-seg is a MSAF dependency, it should be relatively straightforward to include it in the framework. Let me know if I can help with anything in the meantime.

from msaf.

Hi @carlthome , thanks for all the information and the work!

I imagine the version pinning will become immediately problematic however

Okay, I'm on it, I will try to find a good trade-off between the requirements of MSAF and of as-seg!

The as-seg release at PyPI specifies sklearn and not scikit-learn.

Indeed, thanks for pointing it out! On it also.

I will tell you when these changes are pushed in PyPi, should I try to relaunch the install command myself or let you do it?

from msaf.

@carlthome Done!
I modified the dependencies to remove version pinning. For packages that are common between as-seg and MSAF, I set the same minimal versions. I also modified sklearn into scikit-learn. This new version is deployed on PyPi.

Note that I had to specify an upper bound on the version of numpy (<1.24), because there was a conflict between scikit-learn version 0.22.2.post1 and numpy version 1.24.4: the use of np.float in some scikit-learn function (called by librosa.effects) lead to an error in numpy (as in this stack overflow issue).
Analogously, we could update the lower bound for the version of scikit-learn (I was not able to find the minimal version for scikit-learn which fixes this bug, but the latest stable version, 1.3.2, fixes it).

Please tell me if I can do anything else! :)

from msaf.

Great @ax-le! Seems that did the trick, and it's now possible to install as-seg within MSAF: #148

On the NumPy incompatibility, it's been on my radar for a while so thanks for the reminder. New push for resolving that in craffel/mir_eval#366 now.

Please tell me if I can do anything else! :)

The next step would be to call your functions from within a SegmenterInterface. I have a rough idea on how that would look but it's probably best if you try it first since you know the subtleties of your MSA approach the best (I might screw up something crucial, like the 64 sample hop length you've mentioned in your docs, for example).

https://pythonhosted.org/msaf/algorithms.html#adding-a-new-algorithm-to-msaf

If something doesn't fit right (there's always something with software, isn't it? 😄), let's work through needed tweaks in this thread.

Or let me know if you'd prefer that I do a first stab on this and I'll draft a PR.

from msaf.

Hey!
So I have a first draft of the code which should be ok (but I didn't took the time to extensively test it)!
Still, I quickly run into a large problem: a great advantage of our algorithm is the use of a particular pre-processing of data, which necessitates:

1. The computation of features (in particular log mel) with a large hop_length (wich could be feasible as so in MSAF, but would use a lot of memory),
2. The estimation of downbeats (which could be made using the madmom toolbox,
3. The implementation of barwise TF matrices (see [1] for details, or I can give you more detailed references).

In short, this could be tweaked in MSAF, but would require time, and it would be more interesting to properly implement these features in MSAF.

I don't have claims right now to support the use of Barwise TF matrices, but an article with experimental details is accepted and under publication. I think that it would be highly beneficial to implement Barwise TF matrices, but we might probably need to do it together.

So what I propose is 1) that I test the CBM as I have it and make a PR (probably next week), and 2) that we take time to implement Barwise TF matrices.

What do you think?

from msaf.

Closed as it is concerned by PR #153

from msaf.