Mulcat option by JunzheJosephZhu · Pull Request #416 · asteroid-team/asteroid

JunzheJosephZhu · 2021-01-21T17:09:38Z

No description provided.

mpariente · 2021-01-21T17:50:37Z

Great this time, thanks !

mpariente · 2021-01-22T19:42:41Z

@JorisCos do you mind reviewing please?

JorisCos

Thanks for the PR, this looks really nice. A minor fix on the docs is needed otherwise LGTM

JorisCos · 2021-02-01T09:53:49Z

asteroid/masknn/recurrent.py

+        self.intra_linear = nn.Linear(self.intra_RNN.output_size, in_chan)
        self.intra_norm = norms.get(norm_type)(in_chan)
+
        # InterRNN block and linear projection layer (uni or bi-directional)


This comment should be before self.inter_RNN = DoubleRNN(...)

JorisCos · 2021-02-01T09:53:53Z

asteroid/masknn/recurrent.py

+            self.inter_RNN = SingleRNN(
+            rnn_type, in_chan, hid_size, num_layers, dropout=dropout, bidirectional=bidirectional
+            )
        # IntraRNN and linear projection layer (always bi-directional)


mpariente

Let's think about a way which doesn't copy paste the SingleRNN code in DoubleRNN.

asteroid/masknn/recurrent.py

mpariente · 2021-02-01T15:08:36Z

asteroid/masknn/recurrent.py



+class DoubleRNN(nn.Module):
+    """Module for a RNN block.


Need to be more explicit.

mpariente · 2021-02-07T19:35:45Z

@JunzheJosephZhu ?

JunzheJosephZhu · 2021-02-07T20:52:25Z

I guess the best way to do it would be combining them into s single object, adding an option for single/doubleRNN and adding a switch statement into the code?

For example,
class singleRNN(...):
def init(...,...,mulcat=False)
If mulcat:
self.inter_RNN = ...
else:
self.inter_RNN1 =
self.inter_RNN2 =

The code would be somewhat more confusing to readers though

asteroid/masknn/recurrent.py

mpariente · 2021-02-08T07:55:27Z

Let's keep the code like this for now.
Please

Rename DoubleRNN to MulCatRNN
Add a description in the docstring.
Test the mul_cat option in jit_test.py

JunzheJosephZhu · 2021-02-09T03:27:26Z

It looks like when running the jit test, sudormrf always fails the test. Is this something I need to worry about, or is this unrelated to DPRNN?
Also, there's this super weird issue:

The two lines below would work with torch jit:
return torch.cat((rnn_output1 * rnn_output2, inp), 2)
return torch.cat((rnn_output1, inp), -1)

but this line throws an error:
return torch.cat((rnn_output1 * rnn_output2, inp), -1)
E torch.jit.TracingCheckError: Tracing failed sanity checks!
E Encountered an exception while running the trace with test inputs.
E Exception:
E The following operation failed in the TorchScript interpreter.
E Traceback of TorchScript (most recent call last):
E RuntimeError: vector::_M_range_check: __n (which is 18446744073709551615) >= this->size() (which is 3)

I ended up with:
return torch.cat((rnn_output1 * rnn_output2, inp), 2)
which works fine

mpariente

We are getting much closer, thanks ! What's left

Add use_mulcat is get_config so that serialization will work
Add the use_mulcat parameterize in tests/models/models_test.py to ensure serialization will work.
Fix the jit tests which don't look right to me.
Add the reference to the original MulCat paper in MulCatRNN

asteroid/masknn/recurrent.py

tests/jit/jit_models_test.py

JunzheJosephZhu · 2021-02-09T09:26:31Z

Also, do you want to implement multi-stage loss? That was also in the facebook paper.
I personally would prefer to first include it in my own recipe, since I'm gonna re-write the backbone anyway.

mpariente · 2021-02-09T09:38:06Z

Eventually, yes, but in the recipe sounds fine.

since I'm gonna re-write the backbone anyway.

What do you mean?

JunzheJosephZhu · 2021-02-09T09:40:45Z

I mean I will use DPRNNBlock but not DPRNN for my separator, since the part starting from Conv2D diverge.

mpariente · 2021-02-09T09:55:18Z

Alright.
Can you fix the other issues I mentioned so that we can merge?

JunzheJosephZhu · 2021-02-09T15:15:18Z

Ok, I'll do it ASAP. Btw, how do you write test cases for dataset objects? Can I just write if name == "main" in my recipe?

mpariente · 2021-02-09T15:40:47Z

We don't have tests for datasets.. Which is a large limitation.

JunzheJosephZhu · 2021-02-10T08:18:28Z

I just finished my work with variable speaker wsj0mix dataset in my recipe folder. Should I start a PR yet?

This reverts commit 865c401.

tests/models/models_test.py

JunzheJosephZhu · 2021-02-10T09:20:41Z

I have no clue why it's not passing the test_save_and_load_dprnn. Do you have any idea?

mpariente · 2021-02-10T09:21:01Z

The loading is failing.

We need to test the mulcat version, but still keep the old tests. For that, we can use @pytest.mark.parameterize.

Regarding the recipe, yes, you can make a PR 😃 Thanks

mpariente · 2021-02-10T09:21:55Z

Because use_mulcat is not returned in get_config (as I mentioned before) so the new model doesn't use it.

JunzheJosephZhu · 2021-02-10T09:23:45Z

ok

"

This reverts commit d4c4b85.

add to recipe

JunzheJosephZhu · 2021-02-10T11:00:38Z

Hi, I'm starting to write the model class. I noticed that in all your models that are in the source code just use make_optimizer, but those in the recipe define make_model_and_optimizer. Do I have to follow this?
Also, why do the models in the source code not have load_best_model function?

mpariente · 2021-02-10T11:22:05Z

No, you don't have to.
The most important is that loading the model for eval is easy.

JunzheJosephZhu · 2021-02-12T10:29:37Z

how do i write test cases for models in my recipe?
Also, github send me an email everyday telling me the version consistency check failed. I have no clue what it means

mpariente · 2021-02-12T14:02:34Z

how do i write test cases for models in my recipe?

They aren't tested if they are in the recipe folder.

Also, github send me an email everyday telling me the version consistency check failed. I have no clue what it means

Sorry about that, I don't receive those e-mails.. Anybody else receives those e-mails?

asteroid/masknn/recurrent.py

mpariente · 2021-02-12T14:59:37Z

I

Fixed the jit tests
Parameterize model test with use_mulcat
Restore always bidirectional IntraRNN
Removed the recipe files (which should be added to another branch).

This is ready for merging on my side.
Next time, don't make PRs from your master branch please 😉

JunzheJosephZhu · 2021-02-13T03:22:35Z

---------- Forwarded message --------- From: Joseph <notifications@github.com> Date: Sat, Feb 13, 2021 at 8:11 AM Subject: [JunzheJosephZhu/asteroid] Run failed: Version consistency - master (c6ef670) To: JunzheJosephZhu/asteroid <asteroid@noreply.github.com> CC: Ci activity <ci_activity@noreply.github.com> [image: GitHub] [JunzheJosephZhu/asteroid] Version consistency workflow run Version consistency: Some jobs were not successful View workflow run <https://github.com/JunzheJosephZhu/asteroid/actions/runs/562583919> [image: CI (3.6, 1.6.0)] *Version consistency* / CI (3.6, 1.6.0) Cancelled [image: annotations for Version consistency / CI (3.6, 1.6.0)] 2 <https://github.com/JunzheJosephZhu/asteroid/actions/runs/562583919> [image: CI (3.6, 1.7.0)] *Version consistency* / CI (3.6, 1.7.0) Cancelled [image: annotations for Version consistency / CI (3.6, 1.7.0)] 2 <https://github.com/JunzheJosephZhu/asteroid/actions/runs/562583919> [image: CI (3.6, nightly)] *Version consistency* / CI (3.6, nightly) Failed in 1 minute and 47 seconds [image: annotations for Version consistency / CI (3.6, nightly)] 1 <https://github.com/JunzheJosephZhu/asteroid/actions/runs/562583919> — You are receiving this because this workflow ran on your branch. Manage your GitHub Actions notifications <https://github.com/settings/notifications> GitHub, Inc. ・88 Colin P Kelly Jr Street <https://www.google.com/maps/search/88+Colin+P+Kelly+Jr+Street++San+Francisco,+CA+94107?entry=gmail&source=g> ・San Francisco, CA 94107 <https://www.google.com/maps/search/88+Colin+P+Kelly+Jr+Street++San+Francisco,+CA+94107?entry=gmail&source=g>

…

-- Joseph Zhu

mpariente · 2021-02-13T09:49:59Z

Not sure..

Joseph Zhu added 2 commits January 20, 2021 12:17

Mulcat option

fae684d

ran black

8372db7

mpariente requested a review from JorisCos January 21, 2021 17:50

JorisCos reviewed Feb 1, 2021

View reviewed changes

mpariente reviewed Feb 1, 2021

View reviewed changes

Update asteroid/masknn/recurrent.py

3f94979

mpariente reviewed Feb 8, 2021

View reviewed changes

asteroid/masknn/recurrent.py Outdated Show resolved Hide resolved

Update asteroid/masknn/recurrent.py

85435df

Merge remote-tracking branch 'upstream/master'

6ca1bd2

Joseph Zhu added 3 commits February 8, 2021 21:30

minor fixes to doc

4a34d2c

merging files

bebd464

fixed weird issue with jit - negative index does not work with mulcat

c13e6db

mpariente reviewed Feb 9, 2021

View reviewed changes

asteroid/masknn/recurrent.py Outdated Show resolved Hide resolved

tests/jit/jit_models_test.py Outdated Show resolved Hide resolved

mpariente and others added 2 commits February 9, 2021 08:36

Update asteroid/masknn/recurrent.py

bb6d1aa

fix minor issue

93ce3f1

variable speaker wsj0mix

865c401

Revert "variable speaker wsj0mix"

d4c4b85

This reverts commit 865c401.

add reference to mulcat and parameterize in model_test

0dd6182

mpariente reviewed Feb 10, 2021

View reviewed changes

tests/models/models_test.py Outdated Show resolved Hide resolved

Joseph Zhu added 4 commits February 10, 2021 03:27

changed get_config"

0d92876

"

reformat

a3becc6

Revert "Revert "variable speaker wsj0mix""

5ddb4fa

This reverts commit d4c4b85.

Merge branch 'variable-speaker-data'

c8c295e

add to recipe

mpariente added 4 commits February 12, 2021 15:07

IntraRNN is always bidirectional

9d8b33d

Parameterize save and load on use_mulcat

749dec5

No mulcat by default in JIT test

fdfc4fd

Change logic of Models JIT test + test JIT mulcat

2583fb4

mpariente reviewed Feb 12, 2021

View reviewed changes

asteroid/masknn/recurrent.py Outdated Show resolved Hide resolved

mpariente added 2 commits February 12, 2021 15:55

Update asteroid/masknn/recurrent.py

bda4f1a

Remove recipe from this PR

c6ef670

mpariente merged commit 8912fd2 into asteroid-team:master Feb 12, 2021

Conversation

JunzheJosephZhu commented Jan 21, 2021

Uh oh!

mpariente commented Jan 21, 2021

Uh oh!

mpariente commented Jan 22, 2021

Uh oh!

JorisCos left a comment

Choose a reason for hiding this comment

Uh oh!

JorisCos Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JorisCos Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpariente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpariente Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

mpariente commented Feb 7, 2021

Uh oh!

JunzheJosephZhu commented Feb 7, 2021

Uh oh!

Uh oh!

mpariente commented Feb 8, 2021

Uh oh!

JunzheJosephZhu commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpariente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JunzheJosephZhu commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpariente commented Feb 9, 2021

Uh oh!

JunzheJosephZhu commented Feb 9, 2021

Uh oh!

mpariente commented Feb 9, 2021

Uh oh!

JunzheJosephZhu commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpariente commented Feb 9, 2021

Uh oh!

JunzheJosephZhu commented Feb 10, 2021

Uh oh!

Uh oh!

JunzheJosephZhu commented Feb 10, 2021

Uh oh!

mpariente commented Feb 10, 2021

Uh oh!

mpariente commented Feb 10, 2021

Uh oh!

JunzheJosephZhu commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JunzheJosephZhu commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpariente commented Feb 10, 2021

Uh oh!

JunzheJosephZhu commented Feb 12, 2021

Uh oh!

mpariente commented Feb 12, 2021

Uh oh!

Uh oh!

mpariente commented Feb 12, 2021

Uh oh!

JunzheJosephZhu commented Feb 13, 2021 via email

Uh oh!

JorisCos Feb 1, 2021 •

edited

Loading

JorisCos Feb 1, 2021 •

edited

Loading

JunzheJosephZhu commented Feb 9, 2021 •

edited

Loading

JunzheJosephZhu commented Feb 9, 2021 •

edited

Loading

JunzheJosephZhu commented Feb 9, 2021 •

edited

Loading

JunzheJosephZhu commented Feb 10, 2021 •

edited

Loading

JunzheJosephZhu commented Feb 10, 2021 •

edited

Loading