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

Tweak conv-rnn model #75

Merged
merged 4 commits into from
Oct 29, 2017
Merged

Tweak conv-rnn model #75

merged 4 commits into from
Oct 29, 2017

Conversation

daemon
Copy link
Member

@daemon daemon commented Oct 29, 2017

  • Fix misplaced zero_grad()
  • Tweak model hyperparams and optimization algorithm

- Fix misplaced zero_grad()
- Tweak model hyperparams and optimization algorithm
conv_rnn/test.py Outdated
@@ -17,19 +18,19 @@ def main():
parser.add_argument("--gpu_number", default=0, type=int)
args = parser.parse_args()

model.set_seed(5, no_cuda=args.no_cuda)
data_loader = data.SSTDataLoader(args.data_dir)
model.set_seed(3, no_cuda=args.no_cuda)
Copy link
Member

Choose a reason for hiding this comment

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

make seed configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the seed is useless in test.py, I'll remove it later.

@@ -34,8 +34,6 @@ def __init__(self, word_model, **config):
else:
raise ValueError("RNN type must be one of LSTM or GRU")
self.conv = nn.Conv2d(1, n_fmaps, (1, self.hidden_size * 2))
if dropout:
Copy link
Member

Choose a reason for hiding this comment

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

does this hurt performance? if we are removing it why are we adding --dropout_prob to train.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Paper says no dropout for SST1/2.

@tuzhucheng tuzhucheng merged commit 7957dc7 into castorini:master Oct 29, 2017
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.

2 participants