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

Updating the writeup of the RNN design doc and sequence_decoder #5611

Merged
merged 5 commits into from
Nov 16, 2017

Conversation

kavyasrinet
Copy link

@kavyasrinet kavyasrinet commented Nov 13, 2017

Few comments:

  1. Since I could not edit the image(https://github.com/PaddlePaddle/Paddle/blob/develop/doc/design/ops/images/2_level_rnn.png ) attached:
    We should update the following:
    2-th -> 2-nd/ 2nd/ second
    1-th -> 1-st / 1st/ first
  2. We also need to edit the hyperlinks that point to @Superjom 's repo in the Decoder paragraph in this section: https://github.com/PaddlePaddle/Paddle/blob/develop/doc/design/ops/sequence_decoder.md#usage-in-a-simple-machine-translation-model

@kavyasrinet kavyasrinet changed the title Updating the writeup of the RNN design doc Updating the writeup of the RNN design doc and sequence_decoder Nov 13, 2017
@Superjomn
Copy link
Contributor

Thank you for fixing these mistakes.

for

We should update the following:
2-th -> 2-nd/ 2nd/ second
1-th -> 1-st / 1st/ first

all the PNGs should have a corresponding dot file, that image's dot locates at
https://github.com/PaddlePaddle/Paddle/blob/develop/doc/design/ops/images/2_level_rnn.dot

you can also change it in this PR.

@kavyasrinet
Copy link
Author

Thank you for pointing me to the file. Will update the file in this PR.

@kavyasrinet
Copy link
Author

@Superjom Fixed the dot file, please review when you can.

Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

LGTM

@Superjomn Superjomn merged commit e0e3a8a into PaddlePaddle:develop Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants