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

Stacked Bi-Directional LSTM to be compatible with the Seq2Vec wrapper #2318

Merged
merged 5 commits into from Jan 10, 2019

Conversation

Projects
None yet
2 participants
@apmoore1
Copy link
Contributor

commented Jan 9, 2019

Fixes #2316

I have also added an argument to the Doc String of the constructor to indicate that there is a use_highway argument, I have tested that the Sphinx of this documentation looks ok as well and a screenshot showing this:
sphinx documentation


final_state_tuple = (torch.cat(state_list, 0) for state_list in zip(*final_states))
final_h.extend([final_forward_state[0], final_backward_state[0]])
final_c.extend([final_forward_state[1], final_backward_state[1]])

This comment has been minimized.

Copy link
@joelgrus

joelgrus Jan 9, 2019

Contributor

wouldn't the equivalent of the previous code be something like

final_h.append(torch.cat([final_forward_state[0], final_backward_state[0]], -1))

?

was that wrong? or is this the same and I'm just not seeing it?

This comment has been minimized.

Copy link
@apmoore1

apmoore1 Jan 9, 2019

Author Contributor

That is equivalent to the previous code, however that would not work with the Seq2Vec wrapper/interface as this module is Bi-Directional the final_h and final_c should be of shape (num_layers * num_directions, batch, hidden_size) which the code I have put in the pull request will be where as the previous code and your suggestion I believe would be of shape (num_layers, batch, hidden_size) the place that this would causes an error is in lines 104 to 109 in PytorchSeq2VecWrapper as the module has the bidirectional attribute therefore it would reshape the state incorrectly. Sorry I did not mention this problem in the issue or pull request.

This comment has been minimized.

Copy link
@joelgrus

joelgrus Jan 10, 2019

Contributor

I am a little wary about changing the shape of that state, but it doesn't break any tests and it makes the seq2vecencoder work, so 🤞

joelgrus added some commits Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.