Numpy-style broadcasting for all mathematical functions #1563

gchanan · 2017-05-15T22:25:47Z

all functions mentioned in: implements all functions mentioned in https://github.com/gchanan/pytorch/wiki/Broadcasting-Notes

gchanan · 2017-05-15T22:29:03Z

Here is numpy-style broadcasting for pointwise math and reduction functions.

This also changes the keepdim default from True to False.

I have a longer writeup in the works categorizing the numpy semantics and showing that these are the only backwards incompatible changes necessary to get numpy-style broadcasting (i.e. what's left from here only adds functionality that would currently give you an error).

I'll link the writeup when that's done, but feel free to review before then (although you may wish to hold off on merging).

soumith · 2017-05-16T00:00:47Z

let's aim to merge this after the nips deadline on Friday

ezyang

I love it. I went through and wrote some documentation for the new functions while reading. I didn't do a very careful "is this logic correct" code review.

docs/source/notes/broadcasting.rst

+    # x and y are not broadcastable, because x does not have at least 1 dimension
+    
+    >>> x=torch.FloatTensor(5,1,4,1)
+    >>> y=torch.FloatTensor(3,1,1)


docs/source/notes/broadcasting.rst

+    >>> x=torch.FloatTensor()
+    >>> y=torch.FloatTensor(2,2)
+    # x and y are not broadcastable, because x does not have at least 1 dimension
+    


torch/lib/TH/THStorage.c

-TH_API void THLongStorage_calculateExpandGeometry(long *tensorSizes, long *tensorStrides, long tensorDim, THLongStorage *sizes, long **esz, long **est) {
-  ptrdiff_t ndim = THLongStorage_size(sizes);
-  long numUnsqueezed = ndim - tensorDim;
+TH_API int THLongStorage_inferSize2(THLongStorage *output, long *sizesA, long dimsA, long *sizesB, long dimsB, int raiseErrors) {


torch/lib/TH/THStorage.c

-    expandedSizes[i] = 1;
-    expandedStrides[i] = expandedSizes[i+1] * expandedStrides[i+1];
-  }
+TH_API int THLongStorage_inferExpandGeometry(long *tensorSizes, long *tensorStrides, long tensorDim, THLongStorage *sizes, long **esz, long **est, int raiseErrors) {


torch/lib/TH/generic/THTensor.c

+  return 0;
+}
+
+int THTensor_(expand2)(THTensor *ra, THTensor *rb, THTensor *opa, THTensor *opb, int raiseErrors) {


tools/cwrap/plugins/Broadcast.py

+from string import Template
+
+
+class Broadcast(CWrapPlugin):


test/test_torch.py

+            self.assertEqual(t0.size(), r0.size())
+            self.assertEqual(t1.size(), r1.size())
+
+            # case 4: not broadcastable and not nEleme equal -- tested by test_fallback


torch/lib/TH/THStorage.c

  return 0;
 }

+TH_API int THLongStorage_inferSizeN(THLongStorage *output, int n, long **sizes, long *dims, int raiseErrors) {


gchanan · 2017-05-16T23:07:58Z

@ezyang thank you, I will incorporate these suggestions.

If you are interested in how this works / future plans, you may find these notes interesting: https://github.com/gchanan/pytorch/wiki/Broadcasting-Notes.

albanD · 2017-05-17T15:41:50Z

@gchanan could it be possible to have a global switch (at compile time if not possible at runtime) that would disable the automatic broadcasting? For example that just disable the cwrap plugin?
I (and maybe other people) prefer to use view/expand and not have the broadcast that can hide subtle bugs.

gchanan · 2017-05-17T16:06:16Z

@albanD good question and I understand the motivation for tightly controlling your own code. What I'm not sure about is how to make the distinction between your code and library code (or if that distinction is even well defined).

To support what you want as written, we'd have to ensure that our libraries work both with and without broadcasting, which will greatly increase the maintenance burden. It's also not clear to me the distinction between user and library code is really clear anyway: let's say some NN layer is written in such a way that it e.g. multiples the input tensor by a (4,1) tensor and you pass in a (4) tensor. Was it library or user code that caused broadcasting?

What I have now is a UserWarning if your code does not broadcast, but uses the old (deprecated) 1-d pointwise operations (you previously only needed nElem to match up for many functions). I could also add an optional warning for the backwards-incompatible case, that is, the sizes don't match, so you would have used the 1-d pointwise operations, but you are now broadcasting.

It sounds like what you want is a warning if broadcasting changes the sizes at all (I'd be hestitant to make it an error, not a warning, given the argument about library code above). That shouldn't be too difficult to implement, although I'm not sure how useful it will be if our library code broadcasts a lot (you might get a lot of warnings). What do you think?

albanD · 2017-05-17T16:45:34Z

Ho right I assumed that our library will stay broadcasting free (as it is right now).
I agree that keeping it that way may be a bit more complex for people that want to send new PR and always use broadcasting. But here the change are quite minor as you would just need to slightly adapt new PR and the existing code will not change and work as is.
I guess @soumith and @apaszke will need to decide if the possibility of disabling the broadcasting is worth this maintaining cost ?

pavanky · 2017-05-17T16:59:24Z

What about a per tensor switch instead of a global switch ? This will however increase the complexity in user code.

gchanan · 2017-05-17T17:04:07Z

If you pass in a non-broadcasting tensor to the library, does it switch to a broadcasting tensor? If it doesn't, you have the same issue of the library having to work with both (and having to define in what cases the flag is copied).

gchanan · 2017-05-31T21:26:12Z

I think this is ready for review and now implements all functions mentioned in https://github.com/gchanan/pytorch/wiki/Broadcasting-Notes (i.e. it no longer includes only pointwise mathematical and comparison functions like the title says).

Re: turning off broadcasting, the only case I implemented this for was for copy, which allows you to pass a "broadcast" parameter (default: True). There were a number of places in the code where it was clearly intended to copy tensors as 1-d, which wasn't really true for other functions.

This PR now also includes some warnings that you can enable to detect backwards incompatible changes. In particular:

Setting torch.utils.backcompat.broadcast.warning.enabled=True will cause Python warnings in the case where broadcast occurs but previously 1-d view style pointwise ops occured.
Setting torch.utils.backcompat.keepdim.warning.enabled=True will cause Python warnings in the case where the default value of keepdim is used for 1-d reductions.

I've manually enabled these and verified that the only places these warnings are triggered are in the tests are directly in tests, i.e. there are no library calls (at least that are covered by tests) where we rely on the old behavior. This should make turning on these warnings less noisy.

gchanan · 2017-05-31T22:52:55Z

rebased the commits.

killeent

In the future I think it would be helpful if we could come up with some sort of mechanism for breaking up PRs into smaller chunks, although I'm not blaming you for that @gchanan.

Due to the large scope of this PR, I didn't really look at the logic so much, I guess I'll just have to trust you. I added a few nits and questions where I didn't understand generally why things were done.

docs/source/notes/broadcasting.rst

+
+    >>> x=torch.FloatTensor(5,7,3)
+    >>> y=torch.FloatTensor(5,7,3)
+    # same shapes are always broadcastable


docs/source/notes/broadcasting.rst

+    
+    >>> x=torch.FloatTensor(5,1,4,1)
+    >>> y=torch.FloatTensor(3,1,1)
+    # x and y are broadcastable


docs/source/notes/broadcasting.rst

+Many PyTorch operations support :any:`NumPy Broadcasting Semantics <numpy.doc.broadcasting>`.
+
+In short, if a PyTorch operation supports broadcast, then its Tensor arguments can be
+automatically expanded to be of equal sizes (without making copies of the data).


tools/cwrap/plugins/Broadcast.py

+#                              arguments to broadcast specified argument (usually "self") against
+# [inplace] will generate code for in-place function, which doesn't allow the in-place
+#           argument to be broadcast
+# [fallback] if tensors aren't broadcastable, preserves "element number" pointwise behavior,


tools/cwrap/plugins/Broadcast.py

+#           argument to be broadcast
+# [fallback] if tensors aren't broadcastable, preserves "element number" pointwise behavior,
+#            where only number of elements need to match, and tensors are viewed as 1-dimensional.
+# [dims] if the tensors shouldn't be broadcast to specific tensor or tensors, but a combination


torch/csrc/copy_utils.h

    TensorDst* dst = THPTypeInfo<TensorDst>::cdata(dst_);
    TensorSrc* src = THPTypeInfo<TensorSrc>::cdata(src_);

+    TensorSrc *src_save = src;


torch/csrc/cuda/expand_utils.cpp

@@ -0,0 +1,188 @@
+#include "torch/csrc/cuda/THCP.h"


torch/csrc/expand_utils.h

@@ -0,0 +1,155 @@
+#ifndef THP_EXPAND_UTILS_H


torch/csrc/utils.cpp


 template class THPPointer<THPGenerator>;
+
+static bool backCompatBroadcastWarn = false;


torch/lib/TH/THStorage.c

+  THLongStorage_resize(output, ndim);
+  memcpy(THLongStorage_data(output), expandedSizes, sizeof(long)*ndim);
+  THFree(expandedSizes);
+  return 0;


docs/source/notes/broadcasting.rst

+    # x and y are not broadcastable, because x does not have at least 1 dimension
+    
+    >>> x=torch.FloatTensor(5,1,4,1)
+    >>> y=torch.FloatTensor(3,1,1)


docs/source/notes/broadcasting.rst

+    
+    # but:
+    >>> x=torch.FloatTensor(5,2,4,1)
+    >>> y=torch.FloatTensor(3,1,1)


test/common_nn.py

        module_name='LogSoftmax',
        input_size=(1, 3, 10, 20),
-        reference_fn=lambda i, _: torch.exp(i).div_(torch.exp(i).sum(1).expand_as(i)).log_(),
+        reference_fn=lambda i, _: torch.exp(i).div_(torch.exp(i).sum(1, False).expand_as(i)).log_(),


test/test_torch.py

        sm2 = m2[:, 4]
-        res1 = torchfn(sm1, sm2)
+        # suppress broadcastable warning
+        with warnings.catch_warnings(record=True):


test/test_torch.py

+            dims_full = []
+            ndims = random.randint(1, 4)
+            for _ in range(ndims):
+                dims_full = dims_full + [random.randint(1, 8)]


torch/lib/TH/THStorage.c

+      }
+    }
+
+    expandedSizes[ i ] = max_dim_size;


torch/lib/TH/THStorage.c

+  return 0;
+}
+
+TH_API int THLongStorage_inferExpandGeometry(long *tensorSizes, long *tensorStrides, long tensorDim, THLongStorage *sizes, long **esz, long **est, int raiseErrors) {


torch/lib/TH/generic/THTensor.c

-  THArgCheck(THLongStorage_size(sizes) >= THTensor_(nDimension)(tensor), 1, "the number of sizes provided \
-      must be greater or equal to the number of dimensions in the tensor");
-  THArgCheck(THTensor_(nDimension)(tensor) > 0, 0, "can't expand an empty tensor");
+THTensor* THTensor_(newExpand)(THTensor *tensor, THLongStorage *sizes, int raiseErrors) {


torch/tensor.py

+
+            # reshape batches back into result
+            total_expansion = expand_batch_portion + (self_exp_size[-2], other_exp_size[-1])
+            return self_expanded.bmm(other_expanded).view(*(total_expansion))


torch/utils/backcompat/broadcast/warning/__init__.py

+
+    enabled = property(get_enabled, set_enabled)
+
+sys.modules[__name__] = Warning()


gchanan · 2017-06-05T22:51:09Z

@apaszke I incorporated all your suggestions minus the THLongStorage suggestion (due to the @colesbury comment above) and removing raiseErrors from the expand,expand2,expand3 functions, since that comment thread didn't conclude.

There were also new merge conflicts, so the last commit resolves them via a merge, but let me know if you want me to force push everything via a rebase.

apaszke · 2017-06-06T11:21:13Z

I guess we can leave the THSize change, but it'd be better to remove the error flag.

gchanan · 2017-06-06T22:24:52Z

ok, it took some effort to handle the error cases correctly without leaking memory, but I removed the error flag from THTensor.

1) Line up trailing dimensions in broadcast docs. 2) remove unnecessary expand_as in common_nn test. 3) use view in tensor_str instead of resize_. 4) newExpand remove raiseErrors change. 5) clarify expandedSizes/expandedStrides parameters in inferExpandGeometry. 6) simplify inferSize2/inferSizeN implementations. 7) use new-style classes for warning.

take an error_buffer to return a proper error message while being able to handle memory management correctly from calling function.

…nue.

They weren't documented as having those semantics, but tests on master show they do.

in Broadcast plugin when fallback = false.

soumith · 2017-06-07T23:07:48Z

I fixed the remaining lint and merged this into master!

THIS WAS AWESOME @gchanan

soumith · 2017-06-08T02:32:00Z

i forgot that parts of this PR have TH / THC etc. being touched upon. So I have to do the annoying reverse-merge scheme. So for now I reverted this PR on master (force-pushed, sorry), and I'll merge it in properly tomorrow-ish.

soumith · 2017-06-11T09:38:40Z

this is now properly reverse-merged into master

…40722 Upstream merge april

Skip test_typing to avoid `Error importing plugin "numpy.typing.mypy_plugin": No module named 'numpy.typing.mypy_plugin'` It happens because we have numpy==1.20.3 in some of our images. But `mypy` can be used only witn numpy>=1.21 We have numpy==1.20.3 in our images with python3.9 Will check numpy version in run_tests.py and add test_typing to ROCM_BLOCKLIST if numpy version less then 1.21 Fix ROCm/frameworks-internal#8497

Skip test_typing to avoid `Error importing plugin "numpy.typing.mypy_plugin": No module named 'numpy.typing.mypy_plugin'` It happens because we have numpy==1.20.3 in some of our images. But `mypy` can be used only witn numpy>=1.21 We have numpy==1.20.3 in our images with python3.9 Will check numpy version in run_tests.py and add test_typing to ROCM_BLOCKLIST if numpy version less then 1.21 Fix ROCm/frameworks-internal#8497 (cherry picked from commit 3b54c45)

ezyang reviewed May 16, 2017

View reviewed changes

gchanan mentioned this pull request May 16, 2017

Implement Broadcasting with exact semantics as NumPy #491

Closed

soumith closed this May 19, 2017

soumith reopened this May 19, 2017

soumith closed this May 19, 2017

soumith reopened this May 19, 2017

soumith closed this May 19, 2017

soumith reopened this May 19, 2017

soumith closed this May 19, 2017

soumith reopened this May 19, 2017

soumith closed this May 19, 2017

soumith reopened this May 19, 2017

fmassa mentioned this pull request May 23, 2017

spurious tensor broadcasting, and inconsistent behavior with numpy #1629

Closed

gchanan force-pushed the broadcast_reduce_pointwise branch from 48d91a1 to 6430b6e Compare May 31, 2017 22:52

killeent reviewed Jun 1, 2017

View reviewed changes

soumith changed the title ~~Numpy-style broadcasting for pointwise mathematical and comparison functions~~ Numpy-style broadcasting for all mathematical functions Jun 1, 2017

apaszke reviewed Jun 2, 2017

View reviewed changes

gchanan added 13 commits June 7, 2017 19:07

Move expand_utils-inl.h to generic/ and generate via macros.

6ab9ad2

Change async/broadcast copy arguments to be parsed as ints.

4d7e830

Rename check_fallback to check_backincompat_expand_warn for clarity.

b46f130

Simplify python warning settings and cleanup tests.

c0a8304

Remove raiseErrors from THTensor functions, have THStorage functions

e4c3270

take an error_buffer to return a proper error message while being able to handle memory management correctly from calling function.

Fix Prod backward for broadcasting.

4fc0b89

Renamed masked_copy to masked_scatter in test, fix use of break/conti…

d6a90e3

…nue.

Add dist, atan2, lerp to fallback functions.

a3e8eb8

They weren't documented as having those semantics, but tests on master show they do.

Clarify use of warn vs raise in expand_utils and don't catch exception

fe06aa7

in Broadcast plugin when fallback = false.

Use THPUtils_assert rather than THError in torch/csrc/Module.

f7fe8fc

Rename autograd keepdim tests that now default to True.

a41dd70

Ensure warnings are repeated in python2 for tests.

ca54693

soumith closed this Jun 7, 2017

soumith mentioned this pull request Jun 7, 2017

squeeze dimension after mean / sum #289

Closed

soumith reopened this Jun 8, 2017

soumith force-pushed the broadcast_reduce_pointwise branch from f04b3a1 to ca54693 Compare June 8, 2017 02:32

fmassa mentioned this pull request Jun 8, 2017

torch dot function consistent with numpy #138

Closed

gchanan mentioned this pull request Jun 8, 2017

Add a torch.matmul function and document broadcast behavior of it and delegated functions #1756

Closed

soumith closed this Jun 11, 2017

gchanan mentioned this pull request Aug 9, 2017

Fix typos. #2254

Merged

ducha-aiki mentioned this pull request Sep 21, 2017

Sample code to generate hpatch descriptors not working. DagnyT/hardnet#1

Closed

bheinzerling mentioned this pull request Nov 20, 2017

cosine_similarity should not call squeeze() twice #3797

Closed

jjsjann123 pushed a commit to jjsjann123/pytorch that referenced this pull request Apr 18, 2022

Merge pull request pytorch#1563 from csarofeen/upstream_master_bump_0…

be7b7f0

…40722 Upstream merge april


		template class THPPointer<THPGenerator>;

		static bool backCompatBroadcastWarn = false;


		enabled = property(get_enabled, set_enabled)

		sys.modules[__name__] = Warning()

Numpy-style broadcasting for all mathematical functions #1563

Numpy-style broadcasting for all mathematical functions #1563

Uh oh!

Conversation

gchanan commented May 15, 2017 • edited by soumith Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gchanan commented May 15, 2017

Uh oh!

soumith commented May 16, 2017

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

gchanan commented May 16, 2017

Uh oh!

albanD commented May 17, 2017

Uh oh!

gchanan commented May 17, 2017

Uh oh!

albanD commented May 17, 2017

Uh oh!

pavanky commented May 17, 2017

Uh oh!

gchanan commented May 17, 2017

Uh oh!

gchanan commented May 31, 2017

Uh oh!

gchanan commented May 31, 2017

Uh oh!

killeent left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

gchanan commented May 15, 2017 •

edited by soumith

Loading