Skip to content

Add stft_n_filters to BaseDCUNet#406

Merged
mpariente merged 6 commits intoasteroid-team:masterfrom
JorisCos:DCCRN
Jan 25, 2021
Merged

Add stft_n_filters to BaseDCUNet#406
mpariente merged 6 commits intoasteroid-team:masterfrom
JorisCos:DCCRN

Conversation

@JorisCos
Copy link
Collaborator

@JorisCos JorisCos commented Jan 5, 2021

This PR adds stft_n_filters as mentionned in #404.
It also fixes the defaults parameter of DCUNet to match its paper as mentionned in #400

change default parameter to match DCUNet paper
@mpariente
Copy link
Collaborator

There is a problem with CI (1.7.0 build) that might be related to the stride (changed input shape), could someone have a look?

The other STOI problem on the nightly build should be fixed now.

change defaults parameters of DCCRNet
@JorisCos
Copy link
Collaborator Author

JorisCos commented Jan 8, 2021

We can have a closer look to what's wrong with DCCRNet @jonashaag

@JorisCos JorisCos requested a review from jonashaag January 22, 2021 14:40
@mpariente
Copy link
Collaborator

Could you explain the fix please? What was wrong?

@JorisCos
Copy link
Collaborator Author

Sure, I added stft_n_filters in DCUNet and DCCRNet. I also changed the default values of stft_n_filters stft_n_kernel_size and stft_stride. In the tests the models are initialized with the default values and the tests use hard-coded values tied to those default parameters, so they needed to be changed.

@JorisCos
Copy link
Collaborator Author

JorisCos commented Jan 23, 2021

At first, I was confused by the jit tests on DCCRNet that kept failing, but it was because I didn't change this line
masknet_kwargs.setdefault("n_freqs", stft_n_filters // 2) .

@JorisCos
Copy link
Collaborator Author

Before, because stft_n_filters was equal to stft_kernel_size masknet_kwargs.setdefault("n_freqs", stft_kernel_size // 2) was ok but it had to be changed

@mpariente
Copy link
Collaborator

Makes complete sense, thanks for the explanation!

Copy link
Collaborator

@mpariente mpariente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the required edits on the tests, we'll merge that, thanks again

pass everything as kwargs
@mpariente
Copy link
Collaborator

Thanks again !

@mpariente mpariente merged commit c041060 into asteroid-team:master Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants