Implement ConformerFeedForwardV1 Part #6
Conversation
Judyxujj
requested review from
albertz,
curufinwe,
christophmluscher,
JackTemaki,
mmz33,
Marvin84,
SimBe195,
vieting and
Atticus1806
| from torch import nn | ||
|
|
||
|
|
||
| class ConformerFeedForwardv1(nn.Module): |
There was a problem hiding this comment.
In our earlier implementation in RETURNN-common, we decided to call this ConformerPositionwiseFeedForward. Why this name change?
There was a problem hiding this comment.
The name used here is decided in the last implementation meeting
There was a problem hiding this comment.
So why was it decided that way? Why the name change?
There was a problem hiding this comment.
I think most of the people including me are not aware of the name ConformerPositionwiseFeedForward is used in RETURNN-common. If we want to use the consistent name as in RETURNN-common, I can change the name here
There was a problem hiding this comment.
@mmz33 was working on that, and then me as well. We had lots of earlier discussions on all of this, like naming of modules, etc. So I wonder a bit why we ignore all of that now.
There was a problem hiding this comment.
For reference: rwth-i6/returnn_common#58
Although this was only the starting point. Since then, it was also further developed.
There was a problem hiding this comment.
Thanks for the reference, I have now also made activation configurable as in RETURNN-common
There was a problem hiding this comment.
I would have expected that we stick to our earlier discussions and decisions, and not redo all of those discussions, which are all not really so important anyway.
And introducing inconsistencies to our other existing implementation for no good reason is also not nice.
There was a problem hiding this comment.
@albertz in earlier discussions and decisions nearly no one took part. We specifically started over having as much people involved as possible. I definitely see it as a problem that you can not join the Tuesday meeting, and we should find a solution for that, e.g. have another date.
Also if the discussions are not important, then I do not understand the large number of comments from you here.
There was a problem hiding this comment.
Also if the discussions are not important, then I do not understand the large number of comments from you here.
It should not be inconsistent to the existing implementation.
Also, we already had some experience with the earlier decisions, and some things were changed later on. So now re-deciding that again first takes takes time, and then also ignores that earlier experience.
So, I would stick to the original decisions, and be consistent to the existing implementation, unless there are really good reasons to change some parts.
|
@albertz please stop adding each comment as itself, and collect them in one review and send them at once. |
I think that the way I do it is easier, as you can work already as you go. At least, I would prefer it that way, both as the reviewer, and also as the developer. |
A plus for Nick's approach is that other subscribers don't get spammed with mails for every comment. |
|
Some tests should also be added, similar to #4. |
|
|
|
||
| def __init__( | ||
| self, | ||
| input_dim: int, |
There was a problem hiding this comment.
Please change the input to take some kind of ConformerFeedForwardConfig (c.f. #8 for inspiration) and also define that Config in this file, as agreed on in the last meeting.
There was a problem hiding this comment.
The inputs are defined as Config class now
No description provided.