Initial implementation of quantile operator#39417
Initial implementation of quantile operator#39417heitorschueroff wants to merge 1 commit intopytorch:masterfrom heitorschueroff:quantile
Conversation
💊 CI failures summary and remediationsAs of commit 678b1dc (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 74 times. |
|
Ping me when this is ready for review again, @heitorschueroff |
There was a problem hiding this comment.
1e-03 and 1e-06?
Was there a problem with the defaults?
There was a problem hiding this comment.
I've been struggling to find good numbers for atol and rtol because NumPy promotes float to double and hence has better accuracy. For double it works fine, but for floats I keep failing tests even though the numbers are pretty close to about 5 digits.
There was a problem hiding this comment.
Yes, because NumPy promotes float to double I'm struggling to find numbers that work for the float case.
There was a problem hiding this comment.
This copy is unfortunate. Can it be eliminated? One way to eliminate it most of the time would be to have this function return the reshaped quantiles tensor and have the out variant perform the shape check and copy it into the out_ param, if given. Another option would be to not support an out variant at all. Out is little-used and getting the result shape of this operation correct is tricky, after all.
There was a problem hiding this comment.
I'll make it so only the out variant makes the copy but I agree that this is not optimal. I also don't like how I have to permute the dimensions to follow NumPy shape. I think this would really benefit from a custom cpu/cuda implementation. Maybe a follow-up PR?
mruberry
left a comment
There was a problem hiding this comment.
Looks really good overall. Made a few test and doc suggestions and asked a question about the algorithm.
There was a problem hiding this comment.
I'm a little confused by the "i < j" here. What's that intended to express? Rest of this sentence looks great.
There was a problem hiding this comment.
Same question about "i < j" here. Also you have two periods following "j."
facebook-github-bot
left a comment
There was a problem hiding this comment.
@heitorschueroff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Author: Heitor Schueroff <heitorschueroff@fb.com> Date: Mon Jun 8 09:18:02 2020 -0700
facebook-github-bot
left a comment
There was a problem hiding this comment.
@heitorschueroff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@heitorschueroff merged this pull request in c7798dd. |
Summary: Pull Request resolved: #42755 Attempting to land quantile again after being landed here #39417 and reverted here #41616. Test Plan: Imported from OSS Reviewed By: mruberry Differential Revision: D23030338 Pulled By: heitorschueroff fbshipit-source-id: 124a86eea3aee1fdaa0aad718b04863935be26c7
Summary: Implementing the quantile operator similar to [numpy.quantile](https://numpy.org/devdocs/reference/generated/numpy.quantile.html). For this implementation I'm reducing it to existing torch operators to get free CUDA implementation. It is more efficient to implement multiple quickselect algorithm instead of sorting but this can be addressed in a future PR. Pull Request resolved: pytorch#39417 Reviewed By: mruberry Differential Revision: D22525217 Pulled By: heitorschueroff fbshipit-source-id: 27a8bb23feee24fab7f8c228119d19edbb6cea33
Summary: Pull Request resolved: pytorch#42755 Attempting to land quantile again after being landed here pytorch#39417 and reverted here pytorch#41616. Test Plan: Imported from OSS Reviewed By: mruberry Differential Revision: D23030338 Pulled By: heitorschueroff fbshipit-source-id: 124a86eea3aee1fdaa0aad718b04863935be26c7
Implementing the quantile operator similar to numpy.quantile.
For this implementation I'm reducing it to existing torch operators to get free CUDA implementation. It is more efficient to implement multiple quickselect algorithm instead of sorting but this can be addressed in a future PR.