Computing var/stddev and mean at the same time #18731
Conversation
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot retest this please |
|
(about naming: similar function is called |
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot retest this please |
|
This looks nice. Do you have a rough indication of the relative performance between |
|
@pytorchbot retest this please |
This reverts commit bc1cc26.
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ifedan I think there is an issue from previous reviews that was never fixed: Can we get rid of the Then we can have the no-dim functions work the same way as Tensor sum(const Tensor &self, ScalarType dtype) {
return at::native::sum(self, {}, false, optional<ScalarType>(dtype));
} |
|
@umanwizard I did some regression performance test between previous and current implementation: Results from master: import timeit
SETUP_CODE = '''import torch; a = torch.randn(10000, 10);'''
TEST_CODE = '''torch.std(a, dim=1);'''
timeit.repeat(setup = SETUP_CODE,stmt = TEST_CODE,repeat = 3,number = 10)
[0.04082131403265521, 0.02319741202518344, 0.024329718959052116]
SETUP_CODE = '''import torch; a = torch.randn(10000, 10).cuda();'''
TEST_CODE = '''d1=torch.std(a, dim=1);print(d1[0])'''
timeit.repeat(setup = SETUP_CODE,stmt = TEST_CODE,repeat = 3,number = 10)
[0.013087920029647648, 0.015553846955299377, 0.015307638968806714]Results from new implementation: import timeit
SETUP_CODE = '''import torch; a = torch.randn(10000, 10);'''
TEST_CODE = '''torch.std(a, dim=1);'''
timeit.repeat(setup = SETUP_CODE,stmt = TEST_CODE,repeat = 3,number = 10)
[0.029049404023680836, 0.023604059999343008, 0.02448320999974385]
SETUP_CODE = '''import torch; a = torch.randn(10000, 10).cuda();'''
TEST_CODE = '''d1=torch.std(a, dim=1);print(d1[0])'''
timeit.repeat(setup = SETUP_CODE,stmt = TEST_CODE,repeat = 3,number = 10)
[0.01722334197256714, 0.015372609021142125, 0.01548504998208955] |
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ifedan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: The current variance kernels compute mean at the same time. Many times we want both statistics together, so it seems reasonable to have a kwarg/function that allows us to get both values without launching an extra kernel. Pull Request resolved: pytorch/pytorch#18731 Differential Revision: D14726082 Pulled By: ifedan fbshipit-source-id: 473cba0227b69eb2240dca5e61a8f4366df0e029
| for dim in range(x.dim()): | ||
| for unbiased in [False, True]: | ||
| for keepdim in [False, True]: | ||
| std1, mean1 = torch.std_mean(x, dim=dim, unbiased=unbiased, keepdim=keepdim) |
There was a problem hiding this comment.
do you ever check multiple dims that aren't all dims? the constituent functions support that, right?
|
|
||
| - name: std_mean(Tensor self, IntArrayRef dim, bool unbiased, bool keepdim) | ||
| self: var_std_mean_backward(grads, self, result0, result1, dim, unbiased, keepdim, true) | ||
|
|
There was a problem hiding this comment.
I thought you were going to remove these?
Repeating by previous comment:
but that's very different -- __rpow__ is a real python function, see https://docs.python.org/2.0/ref/numeric-types.html, not a made up function for testing.
I think the issue here is actually that common_methods_invocations.py doesn't work on methods. We should fix that! Can you take a look?
For the purposes of this PR, I'm okay with making a fake method, i.e. _std_mean that just calls your function std_mean and you can write a comment about the situation.
…20650) Summary: Added some extra tests for std_mean and var_mean for multiple dims. Some refactoring of previously created tests based on PR comments: #18731 Pull Request resolved: #20650 Differential Revision: D15396101 Pulled By: ifedan fbshipit-source-id: d15c3c2c7084a24d6cfea4018173552fcc9c03a9
The current variance kernels compute mean at the same time. Many times we want both statistics together, so it seems reasonable to have a kwarg/function that allows us to get both values without launching an extra kernel.