Unify checks for normal#69629
Closed
nkaretnikov wants to merge 1 commit intogh/nkaretnikov/2/basefrom
Closed
Conversation
- Split up checks into multiple macros
- Use them in meta and impl functions for now since either might be called
- Get rid of what appear to be just legacy checks that are irrelevant now
- Update docs to not mention deprecated behavior.
Backward compatibility issues:
- Shapes must match exactly between mean and std (no automatic resizing):
Old:
>>> import torch
>>>
>>> mean = torch.rand(3, 4, 5, device='cpu')
>>> std = torch.rand(3, 4, 1, device='cpu')
>>>
>>> torch.normal(mean=mean, std=std).size()
torch.Size([3, 4, 5])
New:
>>> import torch
>>>
>>> mean = torch.rand(3, 4, 5, device='cpu')
>>> std = torch.rand(3, 4, 1, device='cpu')
>>>
>>> torch.normal(mean=mean, std=std)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: inconsistent tensor, output size ([3, 4, 5]) is not the same as
input size ([3, 4, 1])
- Tensors with the same number of elements are treated as different if their
shapes don't match exactly:
Old:
>>> import torch
>>>
>>> out = torch.rand(2, 3, device='cpu')
>>> mean = torch.rand(2, 3, device='cpu')
>>> std = torch.rand(1, 6, device='cpu')
>>>
>>> torch.normal(out=out, mean=mean, std=std)
<stdin>:1: UserWarning: std and mean have the same number of elements, but are
not broadcastable. This was previously a supported mode of operation, but is
now deprecated and the support will be removed in version 1.6 release. Note
that the current implementation reshapes std to the shape of mean, which may be
incur data copies. Please ensure that std and mean are broadcastable to avoid
these issues. (Triggered internally at
aten/src/ATen/native/DistributionTemplates.h:189.)
tensor([[ 0.3345, 0.6314, 1.0163],
[-0.0397, 0.8128, 1.1436]])
New:
>>> import torch
>>>
>>> out = torch.rand(2, 3, device='cpu')
>>> mean = torch.rand(2, 3, device='cpu')
>>> std = torch.rand(1, 6, device='cpu')
>>>
>>> torch.normal(out=out, mean=mean, std=std)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: The size of tensor a (3) must match the size of tensor b (6) at
non-singleton dimension 1
- An empty tensor is returned instead of an error when mean is a float and std
is an empty tensor:
Old:
>>> import torch
>>>
>>> mean = 4.
>>> std = torch.rand(0, device='cpu')
>>>
>>> torch.normal(mean=mean, std=std)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: min(): Expected reduction dim to be specified for input.numel()
== 0. Specify the reduction dim with the 'dim' argument.
New:
>>> import torch
>>>
>>> mean = 4.
>>> std = torch.rand(0, device='cpu')
>>>
>>> torch.normal(mean=mean, std=std)
tensor([])
[ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit fc4c453 (more details on the Dr. CI page):
🕵️ 6 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
This was referenced Dec 8, 2021
lezcano
reviewed
Dec 9, 2021
Collaborator
lezcano
left a comment
There was a problem hiding this comment.
I really liked the macro approach as it makes everything very readable, and it's clear that they simply encapsulate TORCH_CHECKs!
I left a few minor comments, but overall this PR is looking great, thanks!
Its great to see how much cleaner the code looks by using structured kernels really.
Comment on lines
+7299
to
+7301
| The shapes of :attr:`mean`, :attr:`std`, and :attr:`out` (if present) must | ||
| match exactly. Unless the size of :attr:`out` is zero, then it will be resized | ||
| based on the supplied arguments, which must still match. |
Collaborator
There was a problem hiding this comment.
This is probably not correct. Could it be the case that
Suggested change
| The shapes of :attr:`mean`, :attr:`std`, and :attr:`out` (if present) must | |
| match exactly. Unless the size of :attr:`out` is zero, then it will be resized | |
| based on the supplied arguments, which must still match. | |
| The shapes of :attr:`mean` and :attr:`std` must be broadcastable. |
given that you're using infer_size in the previous PR.
I would not mention the out= argument here, as the behaviour of that argument is the same all across the PyTorch library.
Collaborator
There was a problem hiding this comment.
I see in the example in the OP that the following now breaks:
torch.normal(torch.rand(3, 4, 5), torch.rand(3, 4, 1))This does not seem right, as their shapes are broadcastable. We should support this behaviour.
In fact, I believe the only change needed to support this behaviour is to remove the CHECK_NORMAL_SIZES check. Note that the fact that mean and std are broadcastable is checked within at::infer_size in the meta function, so we don't need to add any additional checks for that.
Collaborator
Author
There was a problem hiding this comment.
i'll look into it and follow up later. wasn't sure what behavior is desirable here
Comment on lines
-7302
to
-7303
| .. note:: When the shapes do not match, the shape of :attr:`mean` | ||
| is used as the shape for the returned output tensor |
Collaborator
There was a problem hiding this comment.
Nice to have this deprecated, even if it was 4 versions later than we said we would deprecate it (cc @mruberry)
Comment on lines
+160
to
+165
| #define CHECK_NORMAL_SIZES(output, input) \ | ||
| TORCH_CHECK( \ | ||
| output.sizes().equals(input.sizes()), \ | ||
| "inconsistent tensor, output size (", output.sizes(), \ | ||
| ") is not the same as input size (", input.sizes(), ")"); | ||
|
|
Collaborator
There was a problem hiding this comment.
It looks like this check is not necessary now, as it should be performed by the structured kernels machinery.
Collaborator
Author
There was a problem hiding this comment.
the reason i have these is because everything calls into template implementations now and these template versions still can be called by someone (e.g., see the rng tests i mentioned in the other pr: aten/src/ATen/test/cpu_rng_test.cpp). i can remove the checks if it's considered okay
Collaborator
There was a problem hiding this comment.
We should modify that code to call into the at::normal functions. We should only avoid calling the at:: API when the performance is crucial, and even then, one needs to be careful about that.
Collaborator
Author
There was a problem hiding this comment.
We should only avoid calling the at:: API when the performance is crucial
could you elaborate on the perf bit?
Collaborator
There was a problem hiding this comment.
PyTorch has a custom dispatcher mechanism. When you call a function in the at:: namespace, this mechanism is activated and it calls the correct backend, e.g. CPU, CUDA, XLA, but also forward or backward AD. This has certain overhead, so sometimes you'll find code that calls directly to the corresponding at::native:: function. Now, most of the time, the correct thing to do is to call the at:: function, but if you are implementing your own backward AD (i.e. it's not a composite kernel) then you should be good to call at::native::. See a non-example of this where I did and shouldn't have called an at::native:: within a forward AD implementation: #63568 (comment)
Comment on lines
+214
to
+216
| CHECK_NORMAL_SIZES(output, mean); | ||
| CHECK_NORMAL_SIZES(output, std); | ||
| CHECK_NORMAL_TENSOR_STD(std); |
Collaborator
There was a problem hiding this comment.
(pretty much) no checks should happen in the _impl side of things. They already happen in the meta part, so this should probably go.
Comment on lines
+195
to
+196
| CHECK_NORMAL_SIZES(output, mean); | ||
| CHECK_NORMAL_STD(std); |
Comment on lines
+204
to
+205
| CHECK_NORMAL_SIZES(output, std); | ||
| CHECK_NORMAL_TENSOR_STD(std); |
| template<template<typename> class normal_kernel, typename RNG> | ||
| Tensor& normal_impl_(Tensor& self, double mean, double std, c10::optional<Generator> gen) { | ||
| TORCH_CHECK(std >= 0.0, "normal_ expects std >= 0.0, but found std=", std); | ||
| CHECK_NORMAL_STD(std); |
Collaborator
There was a problem hiding this comment.
As mentioned in the previous PR, this check should probably go if we go down the road of having this operation implemented as a structured kernel.
Collaborator
Author
There was a problem hiding this comment.
noted, but see above
Collaborator
Author
|
to avoid confusion, will open a new stack to address issues related to BC and broadcasting |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Backward compatibility issues:
Old:
New:
shapes don't match exactly:
Old:
New:
is an empty tensor:
Old:
New: