Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Display activation functions as modules. #4045

Merged
merged 12 commits into from Apr 30, 2020

Conversation

mklimasz
Copy link
Contributor

@mklimasz mklimasz commented Apr 9, 2020

Currently some of the activation functions (and therefore list of activation functions in FeedForward or any custom model with list / lambda based one) wouldn't be displayed in model textual summary (by summary I mean print(model)).

I provide wrapper around lambda based implementations of activation functions (non PyTorch ones) and make corresponding change in FeedForward model.

Example result can be seen in test file.

@mklimasz
Copy link
Contributor Author

mklimasz commented Apr 9, 2020

I'm not sure what is failing (cannot see details). I guess tests are failing (test that broke on my branch also seems to be broken on master when I ran that locally).

@matt-gardner
Copy link
Contributor

Thanks for the PR! You can see the failures by logging into team city as a guest. The failures all come from tests that currently use non-modules to specify the activation. Looks like it's two one-line fixes, here:

input_dim=input_dim, num_layers=1, activations=torch.relu, hidden_dims=output_dim
and here:
feedforward = FeedForward(input_dim=10, num_layers=1, hidden_dims=10, activations="linear")

While you're at it, can you change the name of this class to be TestComposeEncoder?

class TestPassThroughEncoder(AllenNlpTestCase):

You'll also need to run black to pass some of our code formatting CI checks.

@matt-gardner
Copy link
Contributor

Also, can you update the docstring to reflect that the input must be a module, not just a function, and give an example; e.g., torch.nn.ReLU instead of torch.relu.

@bryant1410
Copy link
Contributor

Oh, wow, nice! I opened #4101 and I just see this. Nice that someone's working on it. DIfferent motivation but same patch!

Copy link
Contributor

@bryant1410 bryant1410 left a comment

Choose a reason for hiding this comment

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

LGTM

@dirkgr dirkgr self-requested a review April 24, 2020 22:14
@dirkgr dirkgr linked an issue Apr 24, 2020 that may be closed by this pull request
@mklimasz
Copy link
Contributor Author

Hey guys, can we get this merged?

@epwalsh epwalsh merged commit 7cbeb6c into allenai:master Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FeedForward.activations should be a ModuleList
5 participants