-
Notifications
You must be signed in to change notification settings - Fork 24.9k
Add documentation for FeatureAlphaDropout #36295
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
Add documentation for FeatureAlphaDropout #36295
Conversation
💊 CI failures summary and remediationsAs of commit 7a35e87 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 70 times. |
Note: flake8 is failing because of lint errors in |
Hey @apaszke, could you let me know what you think? :) |
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.
lgtm other than two minor points. please fix. thanks!
Thanks for taking another look! Just updated the PR. Hopefully I got it right this time. :) |
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.
Thank you! :)
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.
oh you need to fix the submodule change
Hey, what does this mean? Edit: I think I figured it out. I really hope I did this correctly... |
2b2203c
to
2bb10b4
Compare
no worries, i fixed it. @pytorchbot merge this please |
@apaszke could you please take a look at this so we can close this out? :) |
@choidongyeon sorry, but I'm not at FB so I can't speed up merging this PR. @soumith? |
There's a test failure (https://circleci.com/gh/pytorch/pytorch/5381793?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link). @choidongyeon could you remove Line 191 in 0c936f9
|
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.
Requesting changes b/c of test failure
Thanks for pointing that out @zou3519 ! |
@choidongyeon I'm seeing some merge conflicts in the tests. This is usually caused by the branch being too far behind master (it look like it's around two days old, which really isn't that old...). Could you rebase onto master and force push to the branch? After a rebase and the tests run I think this should be good to go. I can handle merging this when it's ready. |
@zou3519 Oh oops I merged master instead of rebase, hope that's okay.. Thanks for taking a look! |
@choidongyeon yeah, that's fine. I had some quick comments while reading through the docs; sorry I should have done the read through when I requested changes initially. |
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.
lgtm, let's wait for the tests to pass
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.
@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Probably because it misses |
These changes add documentation for FeatureAlphaDropout, based on a need raised in an issue by @ssnl (Issue #9886). Fixes #37436.