Initial implementation of quantile operator #39417
Conversation
mruberry
added
the
module: numpy
heitorschueroff
requested review from
apaszke,
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
💊 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 |
test/test_torch.py
Outdated
| result = torch.quantile(a, q, dim=dim, keepdim=keepdim).cpu() | ||
| expected = np.quantile(a.cpu().numpy(), q.cpu().numpy(), axis=dim, keepdims=keepdim) | ||
| expected = torch.from_numpy(np.array(expected)).type(result.type()) | ||
| self.assertTrue(torch.allclose(result, expected, rtol=1e03, atol=1e06)) |
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.
aten/src/ATen/native/Sorting.cpp
Outdated
| quantiles = quantiles.permute(numpy_dim_order); | ||
| } | ||
|
|
||
| out.copy_(quantiles.reshape(out_shape)); |
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?
torch/_torch_docs.py
Outdated
| quantile(input, q) -> Tensor | ||
|
|
||
| Returns the q-th quantiles of all elements in the :attr:`input` tensor, doing a linear | ||
| interpolation when the q-th quantile lies between two data points i < j. |
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.
torch/_torch_docs.py
Outdated
|
|
||
| Returns the q-th quantiles of each row of the :attr:`input` tensor along the dimension | ||
| :attr:`dim`, doing a linear interpolation when the q-th quantile lies between two data points | ||
| i < j.. By default, :attr:`dim` is `None` resulting in the :attr:`input` tensor |
There was a problem hiding this comment.
Same question about "i < j" here. Also you have two periods following "j."
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
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
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.