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

Implements two front-ends for acoustic encoders #17

Merged
merged 92 commits into from
Jul 21, 2023

Conversation

christophmluscher
Copy link
Contributor

This is not specific for the Conformer so maybe parts/conformer is not the right place?

@Atticus1806
Copy link
Contributor

Some tests regarding the downsampling shapes (similar to #4) would be nice :)

i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
Copy link
Member

@albertz albertz left a comment

Choose a reason for hiding this comment

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

I think the main problem is that the VGG is applied in the wrong way. Or at least inconsistent to how it usually is done. I don't think this is intended this way.

i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
tests/test_conformer.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
tests/test_conformer.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
tests/test_conformer.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
i6_models/parts/conformer/frontend.py Outdated Show resolved Hide resolved
tests/test_conformer.py Outdated Show resolved Hide resolved
@Judyxujj
Copy link
Contributor

I have two questions here,

  1. the param stride should be added to the Config, since usually we set stride in Conv2d to apply the time subsampling
  2. after the front end convolution models, we need to apply one additional linear layer to project the dimension back to the model size. Should the linear layer be included in the front end, or should it be included in the ConformerBlock? I would prefer the former

@christophmluscher
Copy link
Contributor Author

I have two questions here,

  1. the param stride should be added to the Config, since usually we set stride in Conv2d to apply the time subsampling

already added but not pushed yet :)

  1. after the front end convolution models, we need to apply one additional linear layer to project the dimension back to the model size. Should the linear layer be included in the front end, or should it be included in the ConformerBlock? I would prefer the former

I was thinking about putting this as an option into the frontend: with param int N -> add linear layer with N outputs, None -> no linear layer

@Atticus1806

@christophmluscher
Copy link
Contributor Author

tests fail due to the seq mask update missing. I am not 100% sure how to perform the update in a clean fashion. Is there a PyTorch way to do this?

@albertz
Copy link
Member

albertz commented Jun 21, 2023

tests fail due to the seq mask update missing. I am not 100% sure how to perform the update in a clean fashion. Is there a PyTorch way to do this?

I'm not exactly sure what you refer to. What update do you mean? You mean how to compute the seq mask which is supposed to be returned by the frontend? As I mentioned, you could apply maxpooling with the same striding and kernel size just as the other operations.

@christophmluscher
Copy link
Contributor Author

you could apply maxpooling with the same striding and kernel size just as the other operations.

this would also work for conv layers?

tests/test_conformer.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
tests/test_conformer.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/protocol.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/protocol.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/protocol.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/protocol.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
i6_models/parts/frontend/vgg.py Outdated Show resolved Hide resolved
@christophmluscher
Copy link
Contributor Author

@JackTemaki @albertz

Copy link
Contributor

@JackTemaki JackTemaki left a comment

Choose a reason for hiding this comment

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

I would change the code to only support tuples instead of both tuples and integer, this makes the code more readable and more consistent.

@albertz
Copy link
Member

albertz commented Jul 19, 2023

I would change the code to only support tuples instead of both tuples and integer, this makes the code more readable and more consistent.

I'm not sure I agree. PyTorch itself also supports both. And the more common use case it that the user provides just a single int.

@christophmluscher christophmluscher merged commit 18d3f1a into main Jul 21, 2023
2 checks passed
@christophmluscher christophmluscher deleted the chris-frontend branch July 21, 2023 17:02
@michelwi
Copy link
Contributor

I have two questions here,

  1. the param stride should be added to the Config, since usually we set stride in Conv2d to apply the time subsampling

already added but not pushed yet :)

Was this added in the final version of the PR, I don't seem to find it..

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.

None yet

7 participants