Ticket #12964 (closed defect: fixed)

Opened 12 months ago

Last modified 11 months ago

multiply matrix of matrices by a scalar: boom

Reported by: jhpalmieri Owned by: jason, was
Priority: major Milestone: sage-5.1
Component: linear algebra Keywords:
Cc: Work issues:
Report Upstream: N/A Reviewers: Mike Hansen
Authors: Robert Bradshaw Merged in: sage-5.1.beta6
Dependencies: Stopgaps:

Description (last modified by jhpalmieri) (diff)

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 Download.

Attachments

trac_12964.patch Download (2.3 KB) - added by mhansen 12 months ago.
12964-fix-mul.patch Download (1.6 KB) - added by robertwb 12 months ago.
apply just this patch

Change History

Changed 12 months ago by mhansen

comment:1 Changed 12 months 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 12 months ago by mhansen (previous) (diff)

comment:2 Changed 12 months ago by mhansen

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

comment:3 Changed 12 months ago by mhansen

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

Changed 12 months ago by robertwb

apply just this patch

comment:4 Changed 12 months 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 11 months 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 11 months ago by mhansen

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

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 11 months ago by jhpalmieri

  • Description modified (diff)

comment:8 Changed 11 months ago by jdemeyer

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

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

Thanks, Mike, for revewing this.

Replying to 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?

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

Note: See TracTickets for help on using tickets.