Skip to content
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

Conformer Modules #58

Merged
merged 33 commits into from
Nov 9, 2021
Merged

Conformer Modules #58

merged 33 commits into from
Nov 9, 2021

Conversation

mmz33
Copy link
Member

@mmz33 mmz33 commented Nov 4, 2021

Fix #54.

This is a draft now. It still requires the implementation of MultiHeadSelfAttention from #52.

@mmz33 mmz33 requested review from Atticus1806 and albertz and removed request for Atticus1806 November 4, 2021 10:43
@mmz33
Copy link
Member Author

mmz33 commented Nov 4, 2021

The parameters names still need to be changed to be consistent with #53, #55.

nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/math_.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
@albertz
Copy link
Member

albertz commented Nov 4, 2021

See also my other comments (they don't show up in the "Files changed" view because they got outdated due to the changed indent). Mark them as "resolved" when you resolved them.

@albertz
Copy link
Member

albertz commented Nov 4, 2021

Btw, as usual, also see failing tests.

nn/math_.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
nn/conformer.py Outdated Show resolved Hide resolved
@albertz albertz marked this pull request as ready for review November 9, 2021 19:45
@albertz albertz merged commit 8355dea into main Nov 9, 2021
@albertz albertz deleted the zeineldeen_conformer branch November 9, 2021 19:46
@albertz
Copy link
Member

albertz commented Nov 9, 2021

Merged now. Let's further improve this in later PRs or commits.

@albertz
Copy link
Member

albertz commented Apr 22, 2022

A question on the ConformerConvSubsample: I think this is not exactly described in the original paper, right? So where is it exactly described? Where did you base this on?

@albertz
Copy link
Member

albertz commented Apr 22, 2022

But in the original paper, there were some references to other paper they referred to on the preprocessing. I think it is explained in those other papers, or not?

Also, in your implementation here, you use maxpooling as far as I see. But this is different to ESPnet. In ESPnet, they don't use any pooling but just striding instead. Where do you have the pooling from?

@albertz
Copy link
Member

albertz commented Apr 22, 2022

Also, in ESPnet there are some other variants, including VGG2L (and I think VGG-style was also mentioned in the Conformer paper, or the one it refers to?). VGG2L looks also similar to @christophmluscher 's hybrid baseline?

@albertz
Copy link
Member

albertz commented Apr 23, 2022

Further, in ESPnet (ConformerEncoder), there is pos encoding at the end of the preprocessing block: here

This is usually RelPositionalEncoding (e.g. in train_asr_conformer10_hop_length160).

Then, for the self-attention, it usually uses RelPositionMultiHeadedAttention.

@albertz
Copy link
Member

albertz commented Apr 23, 2022

Also related: espnet/espnet#2816, espnet/espnet#2684

@mmz33
Copy link
Member Author

mmz33 commented Apr 23, 2022

Where do you have the pooling from?

Ok then it is not exactly the same. you are right they use striding instead.

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.

Implement standard Conformer encoder
2 participants