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

Wrong document for dynamic_rnn #27981

Closed
ruanchong opened this issue Apr 19, 2019 · 2 comments
Closed

Wrong document for dynamic_rnn #27981

ruanchong opened this issue Apr 19, 2019 · 2 comments
Assignees
Labels
comp:apis Highlevel API related issues stat:awaiting tensorflower Status - Awaiting response from tensorflower TF 1.13 Issues related to TF 1.13 type:docs-bug Document issues

Comments

@ruanchong
Copy link

ruanchong commented Apr 19, 2019

Please make sure that this is a documentation issue. As per our GitHub Policy, we only address code/doc bugs, performance issues, feature requests and build/installation issues on GitHub. tag:doc_template

System information

Describe the documentation issue

It describes the argument sequence_length as "it's more for performance than correctness", but this is wrong:

  • This argument enables this API to extract the last VALID state of RNN instead of a PADDED time step, so it IS for correctness.
  • This argument CANNOT reduce computation, because even with this argument, computation of new states of RNN cell still occurs. The only difference is that tf.nn.dynamic_rnn chooses to copy through old states instead of keeping the new states, and this leads to even larger computation. So it is NOT for performance.

The initial document is correct, see this commit.

it's more for correctness than performance"

I don't really understand why you replaced the correct document with a wrong one.

We welcome contributions by users. Will you be able to update submit a PR (use the doc style guide) to fix the doc Issue?

@muddham muddham self-assigned this Apr 22, 2019
@muddham muddham added comp:apis Highlevel API related issues TF 1.13 Issues related to TF 1.13 type:docs-bug Document issues labels Apr 22, 2019
@muddham
Copy link

muddham commented Apr 22, 2019

@ruanchong We will take a look at it . Thanks!

@muddham muddham assigned jvishnuvardhan and unassigned muddham Apr 22, 2019
@jvishnuvardhan jvishnuvardhan added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Apr 23, 2019
@zakizhou
Copy link

zakizhou commented May 2, 2019

@ruanchong is correct, and sequence_length indeed is for correctness not for performance, so is mask in tf.keras.layers.RNN.__call__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:apis Highlevel API related issues stat:awaiting tensorflower Status - Awaiting response from tensorflower TF 1.13 Issues related to TF 1.13 type:docs-bug Document issues
Projects
None yet
Development

No branches or pull requests

6 participants