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

SGU and DSGU #1115

Closed
wants to merge 8 commits into from
Closed

SGU and DSGU #1115

wants to merge 8 commits into from

Conversation

unnonouno
Copy link
Member

I implemented SGU and DSGU.
http://arxiv.org/abs/1604.02910

@unnonouno unnonouno added the cat:feature Implementation that introduces new interfaces. label Apr 18, 2016
@unnonouno unnonouno changed the title [WIP] SGU and DSGU SGU and DSGU May 31, 2016
@delta2323
Copy link
Member

This comment about MGU is true of SGU and DSGU, too.

def set_state(self, h):
assert isinstance(h, chainer.Variable)
h_ = h
if self.xp == numpy:
Copy link
Member

Choose a reason for hiding this comment

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

Use is to compare modules.

@delta2323 delta2323 self-assigned this May 31, 2016
return h_t


class StatefulSGU(SGU):
Copy link
Member

Choose a reason for hiding this comment

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

I think stateful RNN should not be a derived class of stateless one because if x is an instance of StatefulGRU, isinstance(x, SGU) equals to True, which is counterintuitive.

Actually, existing RNN units, use base class like LSTMBase and GRUBase. Both stateful and stateless RNNs derive this base class.

Copy link
Member

Choose a reason for hiding this comment

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

@unnonouno Could you consider it again?

This was referenced Jul 30, 2016
@delta2323
Copy link
Member

Same as #1101. How about adding the prefix Stateless to SGU and DSGU?

@delta2323 delta2323 mentioned this pull request Dec 5, 2016
@delta2323
Copy link
Member

Could rename L.SGU and L.DSGU to L.StatelessSGU and L.StatelessDSGU, respectively?

@delta2323
Copy link
Member

delta2323 commented Jul 1, 2017

Also, could you resolve the conflict?

@delta2323 delta2323 added the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Sep 7, 2017
@stale
Copy link

stale bot commented Oct 23, 2017

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 Oct 23, 2017
@stale
Copy link

stale bot commented Nov 22, 2017

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

@stale stale bot closed this Nov 22, 2017
@niboshi
Copy link
Member

niboshi commented Nov 22, 2017

Is it OK to abandon this PR?

@delta2323
Copy link
Member

@unnonouno How do you think of it? I don't think it takes too much time to write it up. If you cannot spare time to work on this, somebody could take it over.

@kmaehashi kmaehashi added this to the Closed issues and PRs milestone Dec 12, 2017
@unnonouno unnonouno deleted the sgu branch December 17, 2017 16:55
hvy pushed a commit that referenced this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature Implementation that introduces new interfaces. st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. stale Not updated for a longer period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants