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.

Sign up for GitHub

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

Jump to bottom

add E-branchformer to i6_models #27

Merged
merged 13 commits into from
Oct 26, 2023
Merged

add E-branchformer to i6_models #27

merged 13 commits into from
Oct 26, 2023

Conversation

Judyxujj
Copy link
Contributor

This PR adds E-branchformer to i6_models

@Judyxujj Judyxujj marked this pull request as draft July 17, 2023 17:25
@Judyxujj Judyxujj marked this pull request as ready for review July 19, 2023 09:32
@Judyxujj Judyxujj requested review from kuacakuaca and michelwi July 20, 2023 09:16
Atticus1806
Atticus1806 requested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@Atticus1806 Atticus1806 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Changes are rather docstring problems instead of conceptional problems.

i6_models/assemblies/e_branchformer/e_branchformer_v1.py Outdated Show resolved Hide resolved
i6_models/assemblies/e_branchformer/e_branchformer_v1.py Outdated Show resolved Hide resolved
i6_models/assemblies/e_branchformer/e_branchformer_v1.py Outdated Show resolved Hide resolved
i6_models/assemblies/e_branchformer/e_branchformer_v1.py Outdated Show resolved Hide resolved
i6_models/assemblies/e_branchformer/e_branchformer_v1.py Outdated Show resolved Hide resolved
i6_models/parts/e_branchformer/cgmlp.py Outdated Show resolved Hide resolved
i6_models/parts/e_branchformer/cgmlp.py Outdated Show resolved Hide resolved
i6_models/parts/e_branchformer/cgmlp.py Outdated Show resolved Hide resolved
i6_models/parts/e_branchformer/merge.py Outdated Show resolved Hide resolved
tests/test_e_branchformer.py Show resolved Hide resolved
mmz33
mmz33 requested changes Jul 31, 2023
View reviewed changes
i6_models/parts/e_branchformer/merge.py Outdated Show resolved Hide resolved
@Judyxujj Judyxujj requested review from Atticus1806 and mmz33 August 10, 2023 08:10
@Judyxujj Judyxujj requested a review from kuacakuaca August 17, 2023 09:06
Atticus1806
Atticus1806 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@Atticus1806 Atticus1806 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your work.

JackTemaki
JackTemaki reviewed Sep 5, 2023
View reviewed changes
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.

Just two small things, I did not verify the actual logic though.

i6_models/parts/e_branchformer/cgmlp.py Show resolved Hide resolved
i6_models/parts/e_branchformer/merge.py Show resolved Hide resolved
@Judyxujj Judyxujj force-pushed the jing-ebranchformer branch from 8d51d17 to dac837c Compare October 6, 2023 09:50
@Judyxujj Judyxujj requested a review from JackTemaki October 6, 2023 09:59
christophmluscher
christophmluscher approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@christophmluscher christophmluscher left a comment

Choose a reason for hiding this comment

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

One small doc comment

i6_models/parts/e_branchformer/merge.py Outdated Show resolved Hide resolved
@christophmluscher
Copy link
Contributor

@mmz33 could you please review? then we can merge :)

@mmz33 mmz33 merged commit 871fdce into main Oct 26, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Atticus1806 Atticus1806 Atticus1806 approved these changes

@mmz33 mmz33 mmz33 approved these changes

@christophmluscher christophmluscher christophmluscher approved these changes

@albertz albertz Awaiting requested review from albertz

@curufinwe curufinwe Awaiting requested review from curufinwe

@SimBe195 SimBe195 Awaiting requested review from SimBe195

@vieting vieting Awaiting requested review from vieting

@michelwi michelwi Awaiting requested review from michelwi

@kuacakuaca kuacakuaca Awaiting requested review from kuacakuaca

@JackTemaki JackTemaki Awaiting requested review from JackTemaki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Judyxujj @christophmluscher @JackTemaki @mmz33 @michelwi @Atticus1806 @kuacakuaca