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.

Sign up for GitHub

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

Jump to bottom

Fix for issue #6521 and issue #6810 #6808

Merged
merged 5 commits into from
Nov 11, 2019
Merged

Fix for issue #6521 and issue #6810 #6808

merged 5 commits into from
Nov 11, 2019

Conversation

euler16
Copy link
Contributor

@euler16 euler16 commented Apr 9, 2019

Thank you for creating a pull request!

Please double-check the following.

@euler16
Copy link
Contributor Author

euler16 commented Apr 9, 2019

This PR fixes the issue raised in issue #6521

@toslunar toslunar added cat:document Documentation such as function documentations, comments and tutorials. to-be-backported Pull request that should be backported. labels Apr 9, 2019
toslunar
toslunar reviewed Apr 9, 2019
View reviewed changes
Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. LGTM

@toslunar toslunar self-requested a review April 9, 2019 10:20
@toslunar
Copy link
Member

toslunar commented Apr 9, 2019

Do you want the example to be reviewed in this PR? If no, could you resend a PR from a new branch pointing 0632fe0?

@euler16
Copy link
Contributor Author

euler16 commented Apr 9, 2019 •
edited
Loading

yeah I am fine with it

@euler16 euler16 changed the title Fix for issue #6521 Fix for issue #6521 and adding example to NStepLSTM Apr 9, 2019
@euler16
Copy link
Contributor Author

euler16 commented Apr 9, 2019

since I am adding this PR as part of my GSoC proposal, I will create a new issue for referencing the example part.

@euler16 euler16 changed the title Fix for issue #6521 and adding example to NStepLSTM Fix for issue #6521 and issue #6810 Apr 9, 2019
@euler16 euler16 mentioned this pull request Apr 9, 2019
Closed
toslunar
toslunar reviewed Apr 10, 2019
View reviewed changes
chainer/links/connection/n_step_lstm.py
>>> import chainer
>>> from chainer import Variable
>>> from chainer import links as L
>>> import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Imports are done here.

chainer/links/connection/n_step_lstm.py
>>> [x.shape for x in xs]
[(4, 2), (4, 2), (4, 2), (4, 2), (4, 2)]
>>> lstm = L.NStepLSTM(n_layers, in_size, out_size, dropout_ratio)
>>> hy, cy, ys = lstm(None, None, xs) # passing no hidden or cell state
Copy link
Member

Choose a reason for hiding this comment

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

The style requires two space before # that starts a comment

Suggested change
>>> hy, cy, ys = lstm(None, None, xs) # passing no hidden or cell state
>>> hy, cy, ys = lstm(None, None, xs) # passing no hidden or cell state

chainer/links/connection/n_step_lstm.py
>>> batch = 5
>>> xs = [Variable(np.random.rand(seq_len, in_size)) for i in range(batch)]
>>> [x.shape for x in xs]
[(4, 2), (4, 2), (4, 2), (4, 2), (4, 2)]
Copy link
Member

Choose a reason for hiding this comment

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

It would present general usage to give xs with different sequence lengths.

@Crissman Crissman added the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Apr 18, 2019
@Crissman
Copy link
Member

@euler16 Can you look at the review?

@Crissman
Copy link
Member

Euler, can you revise this?

@Crissman
Copy link
Member

@euler16, it would be great if you could revise. We'd like to reflect these changes, so if we don't hear from you within a week or so, we'll take over this PR to complete it.

Thanks,

@stale
Copy link

stale bot commented Sep 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Not updated for a longer period of time. label Sep 26, 2019
@stale
Copy link

stale bot commented Oct 26, 2019

This issue is closed as announced. Feel free to re-open it if needed.

@stale stale bot closed this Oct 26, 2019
@toslunar toslunar mentioned this pull request Oct 28, 2019
Merged
@toslunar toslunar reopened this Oct 28, 2019
@stale stale bot removed the stale Not updated for a longer period of time. label Oct 28, 2019
@kmaehashi kmaehashi merged commit 75846ea into chainer:master Nov 11, 2019
@toslunar
Copy link
Member

Thanks, @euler16. Your contribution is merged via #8326.

@toslunar toslunar removed st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. to-be-backported Pull request that should be backported. labels Nov 11, 2019
@kmaehashi kmaehashi added this to the v7.0.0 milestone Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toslunar toslunar toslunar left review comments

Assignees

@Crissman Crissman

Labels
cat:document Documentation such as function documentations, comments and tutorials.
Projects
None yet
Milestone
v7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@euler16 @toslunar @Crissman @kmaehashi