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

pre-trained embedding features #110

Open
bmcfee opened this issue Jul 28, 2019 · 5 comments
Open

pre-trained embedding features #110

bmcfee opened this issue Jul 28, 2019 · 5 comments

Comments

@bmcfee
Copy link
Owner

bmcfee commented Jul 28, 2019

Description

pumpp features currently rely on low-level librosa implementations, but we could also have wrappers for pre-trained feature extractors like openl3 and vggish (the latter as implemented by openmic).

There's some details to work out in terms of standardizing the parameters (hop size, etc).

@bmcfee bmcfee added this to the 0.5.1 milestone Jul 28, 2019
@tomxi
Copy link
Contributor

tomxi commented Aug 10, 2019

Also crema... which I think is gonna be my use case.

Why would this involve an api change?

@bmcfee
Copy link
Owner Author

bmcfee commented Aug 12, 2019

Absolutely. Though with crema, there's a circular dependency problem: crema depends on pumpp, so we can't have pumpp depend on crema.

Probably the best option here is to add a pumpp feature extractor class to each crema model. This should be fairly easy to do. Extending foreign objects (ie from another package) is generally a bad idea, but in this case, we're in control of both packages so it shouldn't be a huge deal.

@tomxi
Copy link
Contributor

tomxi commented Sep 1, 2019

Caveat about wrapping pre-trained embedding in general:
If the model contains things like custom Keras layers that can’t be pickled directly, then the resulting pump object won’t be picklable, which may come as a surprise to Pumpp users... This also means things like parallelization with pump that wraps an un-deserializeable model will break with hard to parse error messages...

@beasteers
Copy link
Contributor

I like the idea of making CremaModel automatically usable as a pumpp. For openl3 and vggish and other models, I feel like it might make sense to have them in pumpp and just not imported by default.

import pumpp
from pumpp.feature.integrations import vggish, openl3, keras
pump = pumpp.Pump(vggish.VGGish(...), openl3.OpenL3(...), keras.H5Model(...))

If we do that, then we don't have to worry about circular dependencies so we could add Crema as well for consistency. Just a thought.

(Or alternatively we can have them imported by default and do:

from pumpp.feature.base import FeatureExtractor

class Crema(FeatureExtractor):
    def __init__(self, name='crema', model_dir='./model', *a, **kw):
        from crema.models.base import CremaModel
        self.model = CremaModel()
        self.model._instantiate(os.path.abspath(model_dir))
        super().__init__(name, *a, **kw)

    def transform_audio(self, y):
        return self.model.outputs(y=y, sr=self.sr)

)
Not sure what the protocol for using models that are outside of the crema repo is tho.

@beasteers
Copy link
Contributor

Actually maybe the best option would be to do what @bmcfee suggested (crema model extending Pump) and then add vggish and openl3 as crema models.

@bmcfee bmcfee removed this from the 0.6.0 milestone Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants