ENH: Adding matmul equivalent of multi_dot (Issue #8719) #10690

SpoorthyBhat · 2018-03-04T12:51:59Z

multi_dot is modified to allow stacks of arrays (i.e. N-D arrays rather than just 2-D arrays) internally using matmul.
The optimum order of multiplication is calculated accordingly.

Allows numpy.asscalar to pass scalars

BUG: fixes numpy#4701 related to numpy.asscalar

multi_dot is modified to allow stacks of arrays (i.e. N-D arrays rather than just 2-D arrays) internally using matmul. The optimum order of multiplication is calculated accordingly.

charris · 2018-03-04T20:17:40Z

This needs to be a new function, we cannot change the behavior of existing functions in this way and maintain backward compatibility. Proposals for new functions should also be discussed on the mailing list before heading off to implement them.

charris · 2018-03-04T20:18:59Z

If the proposal for a new function goes through, it will also need a mention in doc/release/1.15.0-notes.rst.

SpoorthyBhat · 2018-03-05T06:39:56Z

Thanks for the review @charris. I will post this proposal on the mailing list.

mhvk · 2018-03-05T15:47:55Z

@charris - by a change in functionality, do you mean that >2-d arrays no longer raise exceptions but actually return a result? I.e., is the worry that there is code out there that relies on getting an exception? I ask since one should weigh breaking such code that against the benefit of not introducing yet another name.

p.s. One might also deprecate via an argument, say, allow_nd, initially defaulting to False but giving a FutureWarning that the default may change to True.

shoyer · 2018-03-05T19:05:32Z

numpy/linalg/linalg.py

    of the matrices [1]_ [2]_. Depending on the shapes of the matrices,
    this can speed up the multiplication a lot.

    If the first argument is 1-D it is treated as a row vector.
    If the last argument is 1-D it is treated as a column vector.
-    The other arguments must be 2-D.
+    If one of the other arguments is N-D, N>2, it is treated as a stack of matrices and broadcast accordingly


Does this mean that the first and last arguments can't be N-D? Why not? Couldn't these also be treated like stacks of matrices?

shoyer · 2018-03-05T19:06:31Z

numpy/linalg/linalg.py

    of the matrices [1]_ [2]_. Depending on the shapes of the matrices,
    this can speed up the multiplication a lot.

    If the first argument is 1-D it is treated as a row vector.
    If the last argument is 1-D it is treated as a column vector.
-    The other arguments must be 2-D.
+    If one of the other arguments is N-D, N>2, it is treated as a stack of matrices and broadcast accordingly
+    All other arguments must be 2-D.


I find this wording a little confusing now -- what does "all other arguments" now refer to?

I might say:

All arguments other than the first and last must be at least 2-D.

shoyer · 2018-03-05T19:07:37Z

I don't think this is a backwards compatibility break. Every case that would be changed here (ndim > 2) raised an error previously.

SpoorthyBhat · 2018-03-06T04:32:27Z

Thanks, @shoyer. I made the corrections you suggested.

shoyer

This needs test coverage.

shoyer · 2018-03-06T05:11:30Z

numpy/linalg/linalg.py

    # cost1 = cost((AB)C) = a0*a1b0*b1c0 + a0*b1c0*c1
-    cost1 = a0 * b1c0 * (a1b0 + c1)
+    cost1 = a0 * b1c0 * max(dim[0:1]) * (a1b0 + c1 * dim[2])


This indexing isn't right: dim[0:1] only pulls out one element.

When matrices are stacked we want a stack of 1x1 matrices or vectors and not the number of stacks to appear as the second or first dimension.

SpoorthyBhat · 2018-03-17T08:34:14Z

Thanks, @shoyer . I added test coverage.

shoyer · 2018-04-02T22:25:47Z

numpy/linalg/tests/test_linalg.py

        D1d = np.random.random(2)  # 1-D

-        # the result should be a scalar
-        assert_equal(multi_dot([A1d, B, C, D1d]).shape, ())
+        # the result should be a a stack of 1x1 matrices 


This isn't consistent with how multi_dot currently works. We can't change existing behavior, so you will need to change this back.

I would feel more confident in this change in general if you preserved existing tests unchanged and simply added additional tests for higher dimensional inputs.

shoyer · 2018-04-02T22:26:32Z

numpy/linalg/tests/test_linalg.py

-        # the result should be a scalar
-        assert_equal(multi_dot([A1d, B, C, D1d]).shape, ())
+        # the result should be a a stack of 1x1 matrices 
+        assert_equal(multi_dot([A1d, B, C, D1d]).shape, (3, 2, 1, 1))


For this input in particular, I think the correct return shape should be (3, 2).

shoyer · 2018-04-02T22:28:07Z

numpy/linalg/linalg.py

-
-        cost[i, j] = min([
-            cost[prefix] + cost[suffix] + cost_mult(prefix, suffix)
-            for k in range(i, j)])


Please don't remove this. The algorithm is presumably still based on Coram, but with some adaptations? The reference to source material is valuable.

shoyer · 2018-04-02T22:32:31Z

numpy/linalg/tests/test_linalg.py

-        # the result should be a scalar
-        assert_equal(multi_dot([A1d, B, C, D1d]).shape, ())
+        # the result should be a a stack of 1x1 matrices 
+        assert_equal(multi_dot([A1d, B, C, D1d]).shape, (3, 2, 1, 1))

    def test_dynamic_programming_logic(self):


You should add another test like this for higher dimensional inputs.

shoyer · 2018-04-02T22:37:50Z

numpy/linalg/linalg.py

@@ -2536,7 +2528,7 @@ def _multi_dot_matrix_chain_order(arrays, return_costs=False):
            j = i + l
            m[i, j] = Inf
            for k in range(i, j):
-                q = m[i, k] + m[k+1, j] + p[i]*p[k+1]*p[j+1]
+                q = m[i, k] + m[k+1, j] + p[i]*p[k+1]*p[j+1]*max(dim[i:j+1])


I don't think this new expression max(dim[i:j+1]) for calculating the number of repeats in broadcasting is correct.

Consider matrix multiplication between inputs of shape (N, 1, X, Y) and (1, M, X, Y). These inputs would have dim = [N, M], but the number of repeats per broadcasting rules is N*M, not max(N, M).

Something like the utility function _broadcast_shape() from stride_tricks could potentially be helpful for calculating this.

InessaPawson · 2022-02-25T16:36:26Z

@SpoorthyBhat Thank you for working on this! Do you think we could get to the finish line some time soon?

mattip · 2023-03-28T15:45:05Z

This PR is problematic:

conceptually, np.dot and np.matmul handle 3d and larger arrays differently:
```
>>> a = np.zeros([4, 5, 5])
>>> b = np.zeros([1, 5, 5])
>>> np.matmul(a, b).shape
(4, 5, 5)
>>> np.dot(a, b).shape
(4, 5, 1, 5)
```
thus is would be problematic to use matmul to chain arrays in a function called multi_dot.
tests were modified to change behaviour

I think we should close it.

mattip · 2023-03-30T10:08:01Z

Closing, for reasons mentioned above. Please comment or reopen if you disagree.

mattip · 2023-04-03T07:51:16Z

Closing. If you wish to continue work on this, you can cherry-pick the commits onto a new PR.

SpoorthyBhat added 5 commits March 1, 2018 23:18

BUG: fixes numpy#4701 related to numpy.asscalar

a944360

Allows numpy.asscalar to pass scalars

Merge pull request #1 from SpoorthyBhat/Asscalar_fix

3fb0757

BUG: fixes numpy#4701 related to numpy.asscalar

ENH: Adding matmul equivalent of multi_dot (Issue numpy#8719)

5bf7d2f

multi_dot is modified to allow stacks of arrays (i.e. N-D arrays rather than just 2-D arrays) internally using matmul. The optimum order of multiplication is calculated accordingly.

Update linalg.py

47428e9

Update linalg.py

88956ce

charris added 01 - Enhancement component: numpy.lib labels Mar 4, 2018

charris added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 4, 2018

shoyer reviewed Mar 5, 2018

View reviewed changes

SpoorthyBhat added 2 commits March 6, 2018 09:53

Changes in description

9540a72

Update type_check.py

440f2d8

shoyer reviewed Mar 6, 2018

View reviewed changes

SpoorthyBhat added 4 commits March 6, 2018 13:30

Indexing fixed

5ac1f91

Minor fix to maintain shape

2e52bab

When matrices are stacked we want a stack of 1x1 matrices or vectors and not the number of stacks to appear as the second or first dimension.

Test cases added

0fad498

Minot edit

eab7ded

shoyer reviewed Apr 2, 2018

View reviewed changes

mattip added the 55 - Needs work label Oct 16, 2018

Base automatically changed from master to main March 4, 2021 02:04

InessaPawson added the 59 - Needs tests label Jun 24, 2022

mattip closed this Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Adding matmul equivalent of multi_dot (Issue #8719) #10690

ENH: Adding matmul equivalent of multi_dot (Issue #8719) #10690

SpoorthyBhat commented Mar 4, 2018

charris commented Mar 4, 2018

charris commented Mar 4, 2018

SpoorthyBhat commented Mar 5, 2018

mhvk commented Mar 5, 2018

shoyer Mar 5, 2018

shoyer Mar 5, 2018

shoyer commented Mar 5, 2018 •

edited

Loading

SpoorthyBhat commented Mar 6, 2018

shoyer left a comment

shoyer Mar 6, 2018

SpoorthyBhat commented Mar 17, 2018

shoyer Apr 2, 2018 •

edited

Loading

shoyer Apr 2, 2018

shoyer Apr 2, 2018

shoyer Apr 2, 2018

shoyer Apr 2, 2018

InessaPawson commented Feb 25, 2022

mattip commented Mar 28, 2023

mattip commented Mar 30, 2023

mattip commented Apr 3, 2023

ENH: Adding matmul equivalent of multi_dot (Issue #8719) #10690

ENH: Adding matmul equivalent of multi_dot (Issue #8719) #10690

Conversation

SpoorthyBhat commented Mar 4, 2018

charris commented Mar 4, 2018

charris commented Mar 4, 2018

SpoorthyBhat commented Mar 5, 2018

mhvk commented Mar 5, 2018

shoyer Mar 5, 2018

Choose a reason for hiding this comment

shoyer Mar 5, 2018

Choose a reason for hiding this comment

shoyer commented Mar 5, 2018 • edited Loading

SpoorthyBhat commented Mar 6, 2018

shoyer left a comment

Choose a reason for hiding this comment

shoyer Mar 6, 2018

Choose a reason for hiding this comment

SpoorthyBhat commented Mar 17, 2018

shoyer Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

shoyer Apr 2, 2018

Choose a reason for hiding this comment

shoyer Apr 2, 2018

Choose a reason for hiding this comment

shoyer Apr 2, 2018

Choose a reason for hiding this comment

shoyer Apr 2, 2018

Choose a reason for hiding this comment

InessaPawson commented Feb 25, 2022

mattip commented Mar 28, 2023

mattip commented Mar 30, 2023

mattip commented Apr 3, 2023

shoyer commented Mar 5, 2018 •

edited

Loading

shoyer Apr 2, 2018 •

edited

Loading