Implement np.diff for single order differences
#50569
Conversation
np.diffnp.diff for single order differences
40c1a1b to
1479088
Compare
There was a problem hiding this comment.
This looks like a good start. As discussed offline, supporting only n=1 is OK for now.
This PR does need to do two things, however:
- more testing, comparing directly against np.diff, and including noncontiguous and inputs that use different dimensions of the input
- documentation
5f7f572 to
af9cc13
Compare
|
cc @bhosmer If I'm planning to support scalars in addition to Tensors for the append and prepend arguments, it seems like one way to do this is to have 4 variants in |
44c01f9 to
07a1d46
Compare
07a1d46 to
5e99c87
Compare
|
[edit: ah you've updated since my last refresh, comment below may be out of date]
Hi @soulitzer - unfortunately I think enumerating the scalar overloads is your only option presently - you may have already looked at these, but just in case, you can search for ".Scalar(" in As far as why there's no more compact alternative, I don't know the full history but I'd guess it's because the only alternative that would spare you the additional C++ entry points (i.e., that wouldn't just be sugar in Hey though, here's a pitch for something else: not using Basically, without it, the kernel signatures can match the declared schema more faithfully. In the case of
Here's a patch that does it (as of your last commit yesterday, anyway - changes are pretty straightforward though) https://gist.github.com/bhosmer/7b3ddfdaa19ccdd474b4a95a7cb44d3b (But feel free not to use if you'd rather stick with what you've got - we're planning to do a big port to get rid of hacky_wrapper later in the half, and it was a good opportunity to work through a concrete use case regardless.) cc @smessmer re optional<Tensor> discussion |
fcd2e54 to
c948f1d
Compare
|
@bhosmer its good to see the @mruberry I think its ready for another look, when you have the chance. Let me know what your thoughts are on introducing the issue in the C++ api, is avoiding this worth adding 18 different overloads (edit: in addition)? |
aten/src/ATen/native/ReduceOps.cpp
Outdated
|
|
||
| Tensor diff_scalar_scalar(const Tensor& self, int64_t n, int64_t dim, c10::optional<Scalar> prepend, c10::optional<Scalar> append) { | ||
| return diff_tensor_tensor(self, n, dim, | ||
| prepend.has_value() ? c10::optional<Tensor>(diff_broadcast_scalar(prepend.value(), self, dim)) : c10::nullopt, |
There was a problem hiding this comment.
@smessmer be nice to have a .map() polyfill for these, any plans? :)
Yeah, you're definitely hitting the most aggravatingly combinatorial weakness of the current setup with a signature like
You could imagine avoiding some of the complexity overhead with some sort of custom Which leaves us with some sort of From my POV this argues for the status quo - or at least, against unions as a way out of it. But cc @smessmer @ezyang @ljk53 for differing POVs, other clever ideas, etc. 😁 |
|
Thanks @bhosmer for some very interesting perspective! I didn't think of ScalarOrTensor, but that does seem like it would probably cover the vast majority of the use case of union. If writing the op implementation what you mean by downstream, I actually do think something like ScalarOrTensor would simplify things here. In the case of |
|
Adding @peterbell10 because we did some tricks with C++ argument defaulting to avoid ambiguity before, and we can do it again here, probably. |
Codecov Report
@@ Coverage Diff @@
## master #50569 +/- ##
=======================================
Coverage 80.84% 80.84%
=======================================
Files 1931 1931
Lines 210879 210919 +40
=======================================
+ Hits 170492 170525 +33
- Misses 40387 40394 +7 |
| @@ -1367,6 +1367,16 @@ | |||
| - func: fill_diagonal_(Tensor(a!) self, Scalar fill_value, bool wrap=False) -> Tensor(a!) | |||
| variants: method | |||
|
|
|||
| - func: diff(Tensor self, int n=1, int dim=-1, Tensor? prepend=None, Tensor? append=None) -> Tensor | |||
There was a problem hiding this comment.
cc @rgommers for a possible issue with NumPy's signature
np.diff has both prepend and append default to "no value", which is represented by:
https://github.com/numpy/numpy/blob/1de46fe2feee9d3c500a83331ac9b75af5aef947/numpy/_globals.py#L57
The comment indicates it's intended to be used with "deprecated keywords." However, it appears these defaults come from the PR adding the kwargs: numpy/numpy#8206. @rgommers, seems like the NumPy signature would ideally have prepend=None and append=None?
There was a problem hiding this comment.
That comment is a little misleading, or at least incomplete. _NoValue is mainly used when keywords are added to a function, and the function forwards its keywords somewhere. See for example np.sum which forwards to the .sum method of its input (which can be a torch.Tensor). Adding keywords to the numpy function that are not yet present in all other/downstream libraries implementing the same functionality would then break previously working code. I'll go fix the code comment.
Wherever you see _NoValue, just read None.
The PR gives this example that would not work with prepend=None for object arrays:
>>> data = np.array([2, 3], dtype=object)
>>> np.diff(data, prepend=None)
array([None, 1], dtype=object) # expected
array([1], dtype=object) # actual
A little far-fetched, but unfortunately this works:
>>> np.array(None)
array(None, dtype=object)
Object arrays are very annoying.
test/test_torch.py
Outdated
| contig = _make_tensor(shape, dtype, device) | ||
| _test_diff_numpy(contig) | ||
|
|
||
| non_contig = torch.empty(shape + (2, 2), dtype=dtype)[..., 0] |
There was a problem hiding this comment.
Use make_tensor instead of _make_tensor:
pytorch/torch/testing/_internal/common_utils.py
Line 1482 in 4a2aa0f
Both here on 4152 and on 4149.
There was a problem hiding this comment.
I've replaced _make_tensor with make_tensor on line 4149, but I don't think we can use make_tensor to produce torch.empty for line 4152.
| non_contig = torch.empty(shape + (2, 2), dtype=dtype)[..., 0] | ||
| non_contig = non_contig.select(-1, -1) | ||
| non_contig.copy_(contig) | ||
| self.assertTrue(not non_contig.is_contiguous() or shape == (1,)) |
There was a problem hiding this comment.
Cool testing of both contiguous and discontiguous use cases.
| op=torch.diff, | ||
| dtypes=all_types_and_complex_and(torch.bool, torch.float16, torch.bfloat16), | ||
| sample_inputs_func=sample_inputs_diff, | ||
| test_inplace_grad=False), |
There was a problem hiding this comment.
Not for this PR, but the test for inplace grads should be skipped automatically if the op doesn't have an inplace variant.
| @@ -1917,6 +1949,9 @@ def __len__(self): | |||
| def ident(x): | |||
| return x | |||
|
|
|||
| # Do NOT add to this list. Method tests are being DEPRECATED and replaced by OpInfos. | |||
|
Hey @soulitzer! This looks great. I made a few doc suggestions and a couple requests on test coverage. |
|
Hey @mruberry, I think I've addressed your comments. I think its ready for another look! |
There was a problem hiding this comment.
@soulitzer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@soulitzer merged this pull request in b18eeaa. |
Implements
np.difffor single order differences only:diffand function variant fordiff_outtest_torchMathdispatchUpdate: we only support Tensors for prepend and append in this PR. See discussion below and comments for more details.
Currently there is a quirk in the c++ API based on how this is implemented: it is not possible to specify scalar prepend and appends without also specifying all 4 arguments.
That is because the goal is to match NumPy's diff signature of
diff(int n=1, int dim=-1, Union[Scalar, Tensor] prepend=None, Union[Scalar, Tensor] append)=Nonewhere all arguments are optional, positional and in the correct order.There are a couple blockers. One is c++ ambiguity. This prevents us from simply doing
diff(int n=1, int dim=-1, Scalar? prepend=None, Tensor? append=None)etc for all combinations of {Tensor, Scalar} x {Tensor, Scalar}.Why not have append, prepend not have default args and then write out the whole power set of {Tensor, Scalar, omitted} x {Tensor, Scalar, omitted} you might ask. Aside from having to write 18 overloads, this is actually illegal because arguments with defaults must come after arguments without defaults. This would mean having to write
diff(prepend, append, n, dim)which is not desired. Finally writing out the entire power set of all arguments n, dim, prepend, append is out of the question because that would actually involve 2 * 2 * 3 * 3 = 36 combinations. And if we include the out variant, that would be 72 overloads!With this in mind, the current way this is implemented is actually to still do
diff(int n=1, int dim=-1, Scalar? prepend=None, Tensor? append=None). But also make use ofcpp_no_default_args. The idea is to only have one of the 4 {Tensor, Scalar} x {Tensor, Scalar} provide default arguments for the c++ api, and addcpp_no_default_argsfor the remaining 3 overloads. With this, Python api works as expected, but some calls such asdiff(prepend=1)won't work on c++ api.We can optionally add 18 more overloads that cover the {dim, n, no-args} x {scalar-tensor, tensor-scalar, scalar-scalar} x {out, non-out} cases for c++ api. [edit: counting is hard - just realized this number is still wrong. We should try to count the cases we do cover instead and subtract that from the total: (2 * 2 * 3 * 3) - (3 + 2^4) = 17. 3 comes from the 3 of 4 combinations of {tensor, scalar}^2 that we declare to be
cpp_no_default_args, and the one remaining case that has default arguments has covers 2^4 cases. So actual count is 34 additional overloads to support all possible calls][edit: thanks to #50767 hacky_wrapper is no longer necessary; it is removed in the latest commit]
hacky_wrapper was also necessary here because
Tensor?will cause dispatch to look for theconst optional<Tensor>&schema but also generate aconst Tensor&declaration in Functions.h. hacky_wrapper allows us to define our function asconst Tensor&but wraps it in optional for us, so this avoids both the errors while linking and loading.[edit: rewrote the above to improve clarity and correct the fact that we actually need 18 more overloads (26 total), not 18 in total to complete the c++ api]