Migrated functions to tf.keras

[douglas125]

This PR addresses #52 by removing the dependency on keras and switching to tensorflow.keras

Proposed version is 0.1.6 because of pull request #56

In particular, #56 keeps the dependency on keras with from keras.utils.conv_utils import conv_output_length

[XReyRobert]

Hi @douglas125 ,

Edit: In fact this is more likely related to PR #56 I've just tried and got the same results.

Thanks for this, I was impatient to try this out.

Unfortunately I ran into issues, at first I thought it was because I was trying with TF2 but got the same behaviour with TF1.

This is the exact same code first using keras and the other one your tf.keras.

model.add(Melspectrogram(sr=44100, n_mels=n_mels, fmin=fmin,
n_dft=n_fft, n_hop=hop_length, input_shape=input_shape, power_melgram=2.0,
return_decibel_melgram=True, htk=True,
trainable_kernel=True, name='melgram'))

Melspectrogram is wrong, check the following images:

With keras and kapre

With tf.keras and your PR

Librosa :

[douglas125]

I see... there is something unexpected.

I am going to try to see where things are going wrong.

EDIT: It could be useful to add a unittest for correctness

[XReyRobert]

I created this colab to ease environment switching for testing.

It's configured (check "Parameters section") to load this PR and plot charts with kapre and librosa to compare them.

Make sure to "RESET" runtime (not restart) to make sure to use the right modules

Make change in the parameter section. Reset Runtimes. and run all cells
It's already configured to install @58

%tensorflow_version 2.x
use_standalone_keras = False
pip install this PR

https://gist.github.com/XReyRobert/b87fdd8bd91b6d2631921e2c9e2e1bd4

[XReyRobert]

@douglas125 Have you been able to check it out Douglas ?

[douglas125]

@XReyRobert
Not yet, this month was crazy.
But I did write some code to start comparing and try to find where it is going wrong.

[douglas125]

On it now, let's see what happens.

Edit: @XReyRobert-IBM In my tests it seems to be working:

def print_versions():
    print("tensorflow("+tf.__version__+") tensorflow.keras("+tf.keras.__version__+") compat.v1.keras("+tf.compat.v1.keras.__version__+")")
    print ("Keras("+keras.__version__+")")
    print ("Kapre("+kapre.__version__+")")
print_versions()

tensorflow(2.0.0) tensorflow.keras(2.2.4-tf) compat.v1.keras(2.2.4-tf)
Keras(2.2.4-tf)
Kapre(0.1.6)

It looks to me that the notebook you shared has something strange:

x, sr = librosa.load("lyricchords.mp3", sr=44410, mono=True)

and then:

visualise_model(src=x, sr=44100, model=model)

whereas librosa uses the sr variable at all times.

Anyways, please find attached my test notebook (outputs are clear -
Test_spectrogram.txt
rename from .TXT to .IPYNB). Granted, there are some variations in the max values, but I would say nothing major.

[douglas125]

@XReyRobert @keunwoochoi would you mind double-checking?

seems correct to me

[keunwoochoi]

Hey, visually looks good to me. But maybe you could also check using things like numpy.allclose()?

[douglas125]

Sure.

Does librosa compute everything in double precision? I am not sure that the kapre implementation is all double.

Would you say compute allclose() for float32, float64 or both?

It may be that float32 is enough for some applications imo

[keunwoochoi]

librosa

Yes it often returns in double precision probably just because that's how numpy works.

allclose

float32 should be enough.

[douglas125]

The tests still needed to be fixed and the pure keras dependency removed. These seem OK now.

It looks like getting allclose() using librosa with float64 and kapre with float32 is extremely hard.

The image below is a plot of the diff of dB-spectrograms of the test audio included in the tests.

Is this result is satisfactory?

[keunwoochoi]

Hey, without a colormap it's hard to know.
On allclose(), because of their precision difference, you probably wanna pass something like atol=1e-6 (usually eps for float32 is 1e-6 or 1e-7).

On the comment above from two weeks ago - just to confirm, so, are the figures with 'kapre' in the titles ones with your converted code?

[douglas125]

Yes, the figures with kapre in the title are the ones generated with the converted code (this PR).

I think the allclose() is going to be harder than that because it looks like the error adds faster with float32. I will need to think more about how to get the exact same results.

What comes to mind is:

• Comparing with results from current master
• Running kapre in float64 GPU to be able to compare properly with librosa

[keunwoochoi]

Comparing with the current master sounds good to me!

…

On Feb 15, 2020, at 20:48, Douglas Coimbra de Andrade ***@***.***> wrote:  Yes, the figures with kapre in the title are the ones generated with the converted code (this PR). I think the allclose() is going to be harder than that because it looks like the error adds faster with float32. I will need to think more about how to get the exact same results. What comes to mind is: Comparing with results from current master Running kapre in float64 GPU to be able to compare properly with librosa — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[douglas125]

Spectrogram and mel-spec were precomputed using the following config:

tensorflow(1.15.0) tensorflow.keras(2.2.4-tf) compat.v1.keras(2.2.4-tf)
Keras(2.2.5)
Kapre(0.1.3.1)

tests in my PC were with the following configuration:

tensorflow(2.0.0) tensorflow.keras(2.2.4-tf) compat.v1.keras(2.2.4-tf)
Keras(2.2.4-tf)
Kapre(0.1.6) <- just so I knew what was the version

The precomputed values are compared in the tests using assert np.allclose(magnitudes_expected, magnitudes_kapre), as seen in test_time_frequency.py

Tests pass (and the actual diff is 0.0).

@XReyRobert @keunwoochoi I left the less strict comparison with librosa in the test code. I think it might be useful in case the kapre implementation changes.

[kongkip]

conv_output_length can be implemented as an helper function

[douglas125]

conv_output_length can be implemented as an helper function

Would you mind elaborating? I tried to change the smallest number of things that kept the results equal.

Thanks

[keunwoochoi]

I'm lost where conv_output_length could be used, but am sure it is this one in keras.

[keunwoochoi]

Hi @douglas125, thanks for the work. There are multiple issues here and at least one of them is mine.

• You can see that this PR has failed CI test here. I can't quite get why it's not automatically shown on this PR page though.. One thing i'll need to fix.
• On python 2.7, it failed because scikit-learn does not work on python 2.7. scikit-learn is a dependency of librosa which we use, but only in the test. Also another thing i'll need to fix.
• The log on python3.6 would be more relevant to you. (https://travis-ci.org/keunwoochoi/kapre/jobs/651050825) It failed because there's no audio backend such as ffmpeg in the test environment which is a virtual environment of Travis. You might wanna change the audio file into numpy array and let the test do numpy.load as a getaway.
• Another issue is to add non-script files (e.g. wav or npy) into a python package. This can be done by adding package_data in the setup.py. This SO answer would tell you better.

Maybe you wanna use tox to make your local test more similar (if not equivalent) to what's happening on Travis. I'll do my part soon :)

[douglas125]

That's fine, I'll do it:

• save the audio file as a numpy array
• add non-script files into package - is it enough to put tests/filename? (sorry, not so much experience there)

[keunwoochoi]

Could be tricky, I also have done it only once. But by testing it on Tox you’ll be able to see errors if it’s not included properly.

…

On Feb 19, 2020, at 17:44, Douglas Coimbra de Andrade ***@***.***> wrote:  That's fine, I'll do it: save the audio file as a numpy array add non-script files into package - is it enough to put tests/filename? (sorry, not so much experience there) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[douglas125]

Is there a link to see the CI results?

The following changes were made:

• changed WAV to saved numpy .NPZ
• added extra files to package
• added tensorflow 2 as a dependency (pure keras had been removed - maybe this is worth removing and putting only in test too?)
• changed rtol to 1e-2 -> in my GPU the results are EXACTLY the same as the previous version though (like I mentioned, I run current version in Google Colab with tf 1 + keras, saved results and printed in pytest)

Now tests pass on py36 (I don't have python 2 - I don't know many people are using it anyways...)

EDIT: They pass on Travis too for py36, using that same link

Tests pass on tox too with cfg:

envlist = py36

[testenv]
# install pytest in the virtualenv where commands will be executed
deps = pytest
commands =
    pytest

result is

GLOB sdist-make: C:\Users\dougl\Desktop\dev\py\kapre\setup.py
py36 inst-nodeps: C:\Users\dougl\Desktop\dev\py\kapre\.tox\dist\kapre-0.1.6.zip
py36 installed: absl-py==0.9.0,astor==0.8.1,atomicwrites==1.3.0,attrs==19.3.0,audioread==2.1.8,cachetools==4.0.0,certifi==2019.11.28,cffi==1.14.0,chardet==3.0.4,colorama==0.4.3,decorator==4.4.1,future==0.18.2,gast==0.2.2,google-auth==1.11.2,google-auth-oauthlib==0.4.1,google-pasta==0.1.8,grpcio==1.27.2,h5py==2.10.0,idna==2.9,importlib-metadata==1.5.0,joblib==0.14.1,kapre==0.1.6,Keras-Applications==1.0.8,Keras-Preprocessing==1.1.0,librosa==0.7.2,llvmlite==0.31.0,Markdown==3.2.1,more-itertools==8.2.0,numba==0.48.0,numpy==1.18.1,oauthlib==3.1.0,opt-einsum==3.1.0,packaging==20.1,pluggy==0.13.1,protobuf==3.11.3,py==1.8.1,pyasn1==0.4.8,pyasn1-modules==0.2.8,pycparser==2.19,pyparsing==2.4.6,pytest==5.3.5,requests==2.23.0,requests-oauthlib==1.3.0,resampy==0.2.2,rsa==4.0,scikit-learn==0.22.1,scipy==1.4.1,six==1.14.0,SoundFile==0.10.3.post1,tensorboard==2.1.0,tensorflow==2.1.0,tensorflow-estimator==2.1.0,termcolor==1.1.0,urllib3==1.25.8,wcwidth==0.1.8,Werkzeug==1.0.0,wrapt==1.12.0,zipp==3.0.0
py36 runtests: PYTHONHASHSEED='496'
py36 runtests: commands[0] | pytest
================================================= test session starts =================================================
platform win32 -- Python 3.6.6, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: C:\Users\dougl\Desktop\dev\py\kapre
collected 9 items

tests\test_backend.py .....                                                                                      [ 55%]
tests\test_time_frequency.py ..                                                                                  [ 77%]
tests\test_utils.py ..                                                                                           [100%]

================================================== warnings summary ===================================================
.tox\py36\lib\site-packages\tensorflow_core\python\pywrap_tensorflow_internal.py:15
  c:\users\dougl\desktop\dev\py\kapre\.tox\py36\lib\site-packages\tensorflow_core\python\pywrap_tensorflow_internal.py:15: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

tests/test_backend.py::test_mel
tests/test_backend.py::test_filterbank_mel
tests/test_time_frequency.py::test_melspectrogram
  c:\users\dougl\desktop\dev\py\kapre\.tox\py36\lib\site-packages\librosa\filters.py:196: FutureWarning: norm=1 behavior will change in librosa 0.8.0. To maintain forward compatibility, use norm='slaney' instead.
    FutureWarning)

-- Docs: https://docs.pytest.org/en/latest/warnings.html
============================================ 9 passed, 4 warnings in 6.32s ============================================
_______________________________________________________ summary _______________________________________________________
  py36: commands succeeded
  congratulations :)

[keunwoochoi]

Here's the link - https://travis-ci.org/keunwoochoi/kapre . Congrats! py36 test passed on Travis, too. I haven't worked on my part yet. Some thoughts.

• py27 - I've been supporting it but probably it's time to let it go. I'm not using it at all, too. I think I can add this changes on this branch once your work is done.

added tensorflow 2 as a dependency (pure keras had been removed - maybe this is worth removing and putting only in test too?)

Do you think it has to be 2? For some reasons I'm using 1.15 till now and I think it'd work the same; so we can be slightly more generous.

changed rtol to 1e-2

Could you try with a smaller numbers, or some other smaller atol ~= 1e-6 ish? Because that would be more rigorous wrt the property of float32.

Thanks for the work again :)

[douglas125]

Do you think it has to be 2? For some reasons I'm using 1.15 till now and I think it'd work the same; so we can be slightly more generous.

I agree - 1.15 is fine.

Could you try with a smaller numbers, or some other smaller atol ~= 1e-6 ish? Because that would be more rigorous wrt the property of float32.`

Yes. In fact, allclose() tests pass with the default atol=1e-8 (I made the argument explicit now). rtol is the problem - my guess would be the super small values that the Fourier transform yields.

Thanks for the work again :)

My pleasure. This repo is a piece of art for deep learning with audio.

[keunwoochoi]

Nice :) thanks! Merging it now, I'll remove py27 in a separate PR.

[kongkip]

Sorry couldn't help on other parts, I am so held up. Thanks @keunwoochoi

[keunwoochoi]

No problem!