-
Notifications
You must be signed in to change notification settings - Fork 310
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
Support non-linearity function and module for PyTorch Module #928
Conversation
33aa8a3
to
6b5f5c8
Compare
Codecov Report
@@ Coverage Diff @@
## master #928 +/- ##
=========================================
+ Coverage 85.27% 85.3% +0.02%
=========================================
Files 160 160
Lines 7674 7661 -13
Branches 958 955 -3
=========================================
- Hits 6544 6535 -9
Misses 937 937
+ Partials 193 189 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about this PR.
The title suggests that it's fixing a non-linearity in multi_headed_mlp_module (perhaps the default was wrong).
The content seems to rewrite how heads in this module are created, and also adds some features (layer norms, selectable output nonlinearities).
Can you add some context behind the change and explain what it does?
6b5f5c8
to
b7dc753
Compare
I updated the description above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yonghyuc the changes look to be making the format of this module more standard with what Linda wrote for other torch modules originally. I want to know if there is a merit to adding out activation functions to the nn.modulelist as opposed to using the torch.functional non linearity functions directly in the forward method of the module.
One benefit that I see is that if a user wants to quickly verify the structure of a module by printing it out (e.g. a policy) if we use nn.modules for the activation functions and add them to the nn.modulelist, then the nonlinearities will appear in the printed output, but if we use the torch.functional activiation functions, they won't appear when we print out the module. I'm wondering if you, @lywong92, and @krzentner can chime in on this design decision.
0212391
to
0cee99f
Compare
@@ -118,5 +109,15 @@ def forward(self, input_val): | |||
x = input_val | |||
for layer in self._layers: | |||
x = layer(x) | |||
|
|||
return [layer(x) for layer in self._output_layers] | |||
if self._hidden_nonlinearity: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be clear, what I'm trying to avoid is having to manually call the non linearities. I think they should just be defined as computations in the nn.ModuleList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have removed the need for this loop, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we'll add support for both nn.nonlinearities and torch.functional nonlinearities in a future pr.
ea0c0a4
to
02d93f6
Compare
|
||
""" | ||
x = input_value | ||
for layer in self._layers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to replace this function with return self._layers(input_values)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.layers is ModuleList, so I think we need to use for loop here.
(I checked this documentation)
Or we can use Sequential to compute the layers like that.
""" | ||
return self.module(input_value) | ||
|
||
# pylint: disable=missing-return-doc, missing-return-type-doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should actually globally allow not documenting __methods__
? It's fairly obvious what __repr__
, __getstate__
, and __setstate__
do.
In the mean time, yeah. Documenting looks cleaner than locally disabling the rule.
@@ -118,5 +109,15 @@ def forward(self, input_val): | |||
x = input_val | |||
for layer in self._layers: | |||
x = layer(x) | |||
|
|||
return [layer(x) for layer in self._output_layers] | |||
if self._hidden_nonlinearity: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have removed the need for this loop, right?
8cbf97d
to
57abfe1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- I removed ready-to-merge
so you can look at my last comments, but feel free to add it back when you're ready.
@Mergifyio backport release-2019.10 |
57abfe1
to
df123d8
Compare
Command
Hey, we reacted but our real name is @Mergifyio |
df123d8
to
987dbaf
Compare
@Mergifyio backport release-2019.10 |
(cherry picked from commit f58b4bd)
Command
Hey, we reacted but our real name is @Mergifyio |
Throw an error because of using non-linear functions in
torch.nn
.Change into using non-linear funcitons in
torch.nn.functional
ortorch
Previously, MultiHeadedMLPModule assumes that we are using non-linear function in torch.nn.
So we make a computational graph inside of __init__ and pass the type of the non-linear function to ModuleList. (
layer.add_module('non_linearity', hidden_nonlinearity)
)However, for consistency with other modules, I changed the way we use the non-linear functions.
(Similar with
MLPModule
, ) make linear layers only in __init__ and call the non-linear functions in the forward function. (x = self._hidden_nonlinearity(x)
)So now, we need to pass non-linear functions in the torch.nn.functional (ex.
torch.tanh
ortorch.nn.functional.relu
) instead of in the torch.nn (extorch.nn.Tanh
,torch.nn.ReLU
)