-
Notifications
You must be signed in to change notification settings - Fork 146
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
Migrated functions to tf.keras #58
Migrated functions to tf.keras #58
Conversation
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, Melspectrogram is wrong, check the following images: |
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 |
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
https://gist.github.com/XReyRobert/b87fdd8bd91b6d2631921e2c9e2e1bd4 |
@douglas125 Have you been able to check it out Douglas ? |
@XReyRobert |
On it now, let's see what happens. Edit: @XReyRobert-IBM In my tests it seems to be working:
It looks to me that the notebook you shared has something strange:
and then:
whereas librosa uses the sr variable at all times. Anyways, please find attached my test notebook (outputs are clear - |
@XReyRobert @keunwoochoi would you mind double-checking? seems correct to me |
Hey, visually looks good to me. But maybe you could also check using things like numpy.allclose()? |
Sure. Does librosa compute everything in double precision? I am not sure that the kapre implementation is all double. Would you say compute It may be that float32 is enough for some applications imo |
Yes it often returns in double precision probably just because that's how numpy works.
|
The tests still needed to be fixed and the pure keras dependency removed. These seem OK now. It looks like getting 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? |
Hey, without a colormap it's hard to know. 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? |
Yes, the figures with I think the What comes to mind is:
|
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.
|
Spectrogram and mel-spec were precomputed using the following config:
tests in my PC were with the following configuration:
The precomputed values are compared in the tests using 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 |
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 |
I'm lost where |
Hi @douglas125, thanks for the work. There are multiple issues here and at least one of them is mine.
Maybe you wanna use |
That's fine, I'll do it:
|
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.
|
Is there a link to see the CI results? The following changes were made:
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
result is
|
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.
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.
Could you try with a smaller numbers, or some other smaller Thanks for the work again :) |
I agree - 1.15 is fine.
Yes. In fact,
My pleasure. This repo is a piece of art for deep learning with audio. |
Nice :) thanks! Merging it now, I'll remove py27 in a separate PR. |
Sorry couldn't help on other parts, I am so held up. Thanks @keunwoochoi |
No problem! |
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