Opened 6 years ago

Closed 5 years ago

multiply matrix of matrices by a scalar: boom

Reported by: Owned by: jhpalmieri jason, was major sage-5.1 linear algebra sage-5.1.beta6 Robert Bradshaw Mike Hansen N/A

From stackoverflow: Define `a` to be a matrix whose entries are matrices, and multiply `a` by a scalar:

```a = matrix([[matrix([[ 1,  2], [ 3,  4]]),
matrix([[ 5,  6], [ 7,  8]])],
[matrix([[ 9, 10], [11, 12]]),
matrix([[13, 14], [15, 16]])]])
3 * a
```

This results in the following error message:

```TypeError: unsupported operand parent(s) for '*':
'Full MatrixSpace of 2 by 2 dense matrices over Integer Ring' and
'Full MatrixSpace of 2 by 2 dense matrices over Integer Ring'
```

which is clearly nonsense. Multiplying `a` by, for example, `3 * identity_matrix(2)` works, by the way.

Apply only 12964-fix-mul.patch.

comment:1 Changed 6 years ago by mhansen

This happens because matrices don't really use `_mul_`. The end result is less efficient than it could be (since you're doing matrix multiplications for each entry); however, I think this would be for another ticket.

Last edited 6 years ago by mhansen (previous) (diff)

comment:2 Changed 6 years ago by mhansen

• Authors set to Mike Hansen
• Status changed from new to needs_review

comment:3 Changed 6 years ago by mhansen

Another option would be to define `_mul_` on matrices to do `_matrix_times_matrix_`.

Changed 5 years ago by robertwb

apply just this patch

comment:4 Changed 5 years ago by robertwb

My first thought when reading this patch (before even reading your comments) was also to add a _mul_ method on matrices. _rmul_ should always be safe to call with identical ring elements, and this assumption is used in many other places.

comment:5 follow-up: ↓ 9 Changed 5 years ago by jhpalmieri

Robert: after your patch, regarding `__mul__` and `_mul_` for matrices in `element.pyx`: what are their roles? In what order do they get called?

comment:6 Changed 5 years ago by mhansen

• Authors changed from Mike Hansen to Robert Bradshaw
• Reviewers set to Mike Hansen
• Status changed from needs_review to positive_review

The matrix code (changed in the first patch) assumed that you can always call `_mul_` on the entries of the matrix in order to do multiplication. However, this is not the case if the entries of the matrix are in fact matrices themselves since they bypass the coercion system, implement `__mul__` directly (which calls `_matrix_times_matrix_`), and don't implement `_mul_`. Robert's patch just implements `_mul_` to do "what it should" in case there are other places that assume that you can always use `_mul_` directly.

Positive review for Robert's patch.

Apply only 12964-fix-mul.patch .

comment:7 Changed 5 years ago by jhpalmieri

• Description modified (diff)

comment:8 Changed 5 years ago by jdemeyer

• Merged in set to sage-5.1.beta6
• Resolution set to fixed
• Status changed from positive_review to closed

comment:9 in reply to: ↑ 5 Changed 5 years ago by robertwb

Thanks, Mike, for revewing this.

Robert: after your patch, regarding `__mul__` and `_mul_` for matrices in `element.pyx`: what are their roles? In what order do they get called?
To close the loop on this, `a.__mul__(b)` is called by Python when a * b is encountered. `a._mul_(b)` is called by the coercion model when `parent(a) is parent(b)` to actually do the arithmetic. Full details at http://www.sagemath.org/doc/reference/coercion.html