-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 "Classifying Names with a Character-Level RNN" #2954
Conversation
…ain and test sets as well as simplifying content
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/tutorials/2954
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e31a141 with merge base a91f631 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @mgs28! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
@svekars - it looks like you are active a lot in this repo, any chance you could help me with this? Thanks! |
…to mgs28-char-rnn-update
Added functionality to process training data in mini batches to satisfy original story. However, had to use numpy + random to create batch indices from a given dataset. Also, simplified training so it was a closer match to https://pytorch.org/tutorials/beginner/basics/optimization_tutorial.html |
@svekars - would you please help me with this tutorial update pull request or point me to someone who could? |
cc: @spro |
…rs, adding device config for CI steps, cleaning up documentatation
@@ -3,6 +3,7 @@ | |||
NLP From Scratch: Classifying Names with a Character-Level RNN | |||
************************************************************** | |||
**Author**: `Sean Robertson <https://github.com/spro>`_ | |||
**Updated**: `Matthew Schultz <https://github.com/mgs28>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we typically don't add Update. Please remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svekars - anything else I can do to make this better? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svekars - hello, are there other items I should address here? I appreciate your help with this!
Co-authored-by: Svetlana Karslioglu <[email protected]>
@svekars - Sorry on two accounts:
I made one departure from your edits. You suggested removing a comma in "train text and validate our models" but when I re-read it, I actually meant "train, test and validate our models". I updated the text to read as such. I apologize for my delay and muddying up this review but greatly appreciate your feedback on this. All your points were great. Thanks! |
@svekars - Happy Tuesday. Is there anything else I need to do to help merge this tutorial in? Thanks! |
@svekars - Fixed it. Sorry I missed that warning! Thanks |
@mgs28 I really want to merge this but I wonder what happened in the update that increased the time of the build for this tutorial from ~6 minutes to 22 minutes |
@svekars - I also want you to merge it in. :) History: The training is different (batches instead of random samples) and if you look at the comments around July 11th, we tinkered with the parameters to make the confusion matrix better. My best suggestion is to set device = cpu at the beginning of the script. With 10% of my laptop CPU, it takes about 4 minutes. Can you share some of the specs of the deployment machine? That might give me a bit more to dig into. Thanks! |
It's a 1 GPU machine. Can you try with device = cpu? |
@svekars - good idea. I removed the device options at the top of the script. pyspelling passed and the build html looks good. Please let me know if I missed something. Thanks! |
well, now its43 minutes... |
@svekars - well... clearly that's the wrong direction... I submitted a change to speed it back up What's your goal? In July and more recently, I compared the old code to this new addition.
All of these changes make this tutorial more in line with best practices. To make the time goal, I can reduce training by a percentage.
I checked in the 11 minute version for you which I think is a good compromise. Do you agree? |
Agree on the 11 minutes compromise - thank you for adjusting! Please add a note that for the purpose of example the number of epochs is reduced and we recommend to set it to the a larger number for higher level of accuracy. |
@svekars - good idea and done. I added a comment where training is specified. The first item in the end exercises also mentions hyperparameter tuning as a good place to start. Does that work? Thanks! |
Fixes #1166
Description
Updating Sean's excellent RNN classification tutorial that is now 8 years old and missing some newer pytorch functionality.
Checklist
cc @albanD