search

bmcfee / pumpp

practically universal music pre-processor
ISC License
60 stars 11 forks source link

added option to use `tensorflow.keras` #123

Closed beasteers closed 2 years ago

beasteers commented 5 years ago

What does this implement/fix? Explain your changes.

I added a method to build tensorflow.keras from a pumpp.

import tensorflow.keras.layers as tfkl
...
input_ = pump.layers('tf.keras')[INPUT_NAME]
bmcfee commented 5 years ago

Thanks for this one -- I don't think we should / need to invoke environment variables though, since it makes the internal behavior rather obscure and hard to predict. It'd be better if people just change their code to use the appropriate API for their use case.

beasteers commented 5 years ago

Yeah I realized that and overwrote those changes (I think you're looking at an old version of the PR, I force pushed and updated the description a few weeks back)

Instead I just added pump.layers('tf.keras') which fits neatly in how things are currently done. Sorry about the confusion

beasteers commented 5 years ago

Not sure how that old commit slipped by - you were right. Fixed now

beasteers commented 4 years ago

Just thought I'd circle around on this one. It's a small change that adds pump.layers('tf.keras') as an input creation method - I fixed it so it's using the same method you use for creating keras vs tensorflow layers.

amilkh commented 4 years ago

Any update on when this will be merged? https://pypi.org/project/vggish-keras/

beasteers commented 4 years ago

That depends on Brian - he's a pretty busy person so I'm not expecting it soon. But seeing as this PR is still up to date with master, it should be safe for now.

I was working on that package a while back and it seems that for some reason I didn't add this specific branch as an install dependency.

In the most up to date version (0.1.1), it will install pumpp from this branch.

I also cleaned a couple things up and modified the API and README a bit to make things a bit more organized and clearer. So use the readme examples to update existing code (main change is that get_embedding and friends return the timestamps along with the embeddings.)

bmcfee commented 4 years ago

I can probably get back to this project in the next couple of weeks. Got a few other things on my plate right now, but this PR generally looks good. I'll need to get my head back into pumpp for a minute to think it through, but it shouldn't be too long until we merge this. (Maybe a mid-june release?)

bmcfee commented 2 years ago

This build is failing because of the following line, which wasn't tested in main :scream: :

https://github.com/bmcfee/pumpp/blob/4009672af63572b6c19416871ca068513c2940f7/pumpp/feature/base.py#L126-L128

The problem is that in bumping up to tf2, we lose the placeholder api. We can get it back by using v1 compat, eg:

import tensorflow.compat.v1 as tf1
tf1.placeholder(...)

The problem then becomes that this is incompatible with eager execution mode. We can disable this using tf1.disable_eager_execution(), but this produces a side effect that I'm not entirely happy about. AFAIK the recommended solution here (in tf2) is actually to use the keras interface (which works anyway) so I'm not too bothered by it, but it's worth pointing out.

Additionally, we might want to rename the tf options to "tf1" and "tensorflow1" for the 0.6 release.