-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[adaptive_]max_pool{1,2,3}d: handle edge case when input is filled with -inf #40665
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
[adaptive_]max_pool{1,2,3}d: handle edge case when input is filled with -inf #40665
Conversation
💊 CI failures summary and remediationsAs of commit ffd89d8 (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 or post in the (internal) Dr. CI Users group. This comment has been revised 47 times. |
1d51752 to
53dfb79
Compare
|
@xwang233, I didn't know who would be appropriate so I have marked you as a reviewer. I will create the XLA issue after the first review. |
xwang233
left a comment
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.
Thanks for the fix! Overall it looks good. Only the default max index needs to be changed.
|
I'm not familiar with the XLA test. If XLA failure is real, we can temporarily disable test on XLA and leave a TODO there. Seems like |
|
Good call, @xwang233. Fixed |
|
Thanks for the quick and patient reviews, @xwang233. I won't have the time to work on this after Sunday next week (July 19th) so it would be brilliant to get this PR merged before then. |
|
@Baranowski Thanks. I don't work at FB, so I can't merge this for you. |
|
It would be good to get a performance comparison before and after here. |
facebook-github-bot
left a comment
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.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I have added benchmarks in the most recent commit. I don't see significant differences. Master: DetailsModified version: Details |
This reverts commit 3b08ca8f6ddbddefab0088d135c54bb29f25a130.
This reverts commit 49d166cced57fe048138e57a3612d2a02288d61c.
This reverts commit 302276ced0a8b2f05fc12c7dde8b1ee642088b8f.
fe71925 to
ffd89d8
Compare
facebook-github-bot
left a comment
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.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Fixes #40131