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

tf.contrib.rnn.GLSTMCell is hilariously broken #16703

Closed
akhti opened this issue Feb 2, 2018 · 6 comments
Closed

tf.contrib.rnn.GLSTMCell is hilariously broken #16703

akhti opened this issue Feb 2, 2018 · 6 comments
Assignees
@asimshankar

Comments

@akhti
Copy link

akhti commented Feb 2, 2018

In 3f57902 an "if" was added that caches linear transformation weights:
https://github.com/tensorflow/tensorflow/blob/r1.5/tensorflow/contrib/rnn/python/ops/rnn_cell.py#L2316

The problem is that this _linear is inside a loop. And so the change tied weights of all these linear transformations.

CC @okuchaiev
@reedwm
Copy link
Member

reedwm commented Feb 2, 2018

/CC @panyx0718, can you take a look?

@reedwm reedwm added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Feb 2, 2018
@panyx0718
Copy link

Right. this is a bug introduced during a refactoring. Perhaps we should cache one Linear for each group_id.

@reedwm
Copy link
Member

reedwm commented Feb 2, 2018

/CC @asimshankar, can you fix this?

@asimshankar
Copy link
Contributor

Doh! Thanks for pointing this out @akhti - will send out a fix.
(Though, not sure if this will result in an update to the 1.5 release since APIs in tf.contrib are experimental and bugs there aren't treated with the same urgency as the stable APIs).

@asimshankar asimshankar self-assigned this Feb 3, 2018
@akhti
Copy link
Author

akhti commented Feb 3, 2018 •
edited

No worries - the bug was here since October and nobody noticed :)
Though it's a very nasty bug - nothing fails, but model quality is suboptimal. Is it possible to add some simple test, like for a number of parameters to avoid this in the future?

okuchaiev added a commit to okuchaiev/tensorflow that referenced this issue Feb 6, 2018
@okuchaiev
Fix for GLSTMCell implementation
172788f 
* Fix issue described here: tensorflow#16703
* Make it work correctly for input sizes!=num_units
@okuchaiev okuchaiev mentioned this issue Feb 6, 2018
Closed
@okuchaiev
Copy link
Contributor

Please have a look at this PR #16788

@tensorflowbutler tensorflowbutler removed the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Feb 6, 2018
@case540 case540 closed this as completed in d0904cb Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@asimshankar asimshankar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@asimshankar @panyx0718 @reedwm @akhti @okuchaiev @tensorflowbutler