Opened 8 months ago

Closed 8 months ago

#31234 closed defect (fixed)

multiplication of matrix with zero columns or rows fails

Reported by: gh-mwageringel Owned by:
Priority: major Milestone: sage-9.3
Component: linear algebra Keywords: coercion
Cc: Merged in:
Authors: Travis Scrimshaw Reviewers: Markus Wageringel
Report Upstream: N/A Work issues:
Branch: 2fefdb1 (Commits, GitHub, GitLab) Commit: 2fefdb156e00dcc589e3b48e7de17d8fd81aa223
Dependencies: Stopgaps:

Status badges

Description

For matrices with zero columns or rows and different base rings (at least when double fields are involved), multiplication fails:

sage: matrix.identity(QQ, 4) * matrix(RDF, 4, 0)
...
ValueError: matrix is immutable; please change a copy instead (i.e., use copy(M) to change a copy of M).

sage: matrix.identity(RDF, 4) * matrix(QQ, 4, 0)
...
ValueError: matrix is immutable; please change a copy instead (i.e., use copy(M) to change a copy of M).

sage: matrix(QQ, 0, 4) * matrix.identity(RDF, 4)
...
ValueError: matrix is immutable; please change a copy instead (i.e., use copy(M) to change a copy of M).

Using copy(), as suggested, does not work.

Change History (8)

comment:1 follow-up: Changed 8 months ago by chapoton

At least one issue in change_ring about subdivisions:

sage: A = matrix.identity(QQ, 4)                                                
sage: A._subdivisions                                                           
sage: A2 = A.change_ring(RDF)                                                     
sage: A2._subdivisions                                                          
( [ 0, 4 ], [ 0, 4 ] )
Version 0, edited 8 months ago by chapoton (next)

comment:2 follow-up: Changed 8 months ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/matrix/double_zero_matrix_mult-31234
  • Commit set to 214e9b4bbced06cd0a09dc21f47f606a110f62e5
  • Status changed from new to needs_review

I have found the problem. In matrix_double_dense.pyx, the corresponding _matrix_times_matrix_ has the following:

        if self._nrows == 0 or self._ncols == 0 or right._nrows == 0 or right._ncols == 0:
            return self.matrix_space(self._nrows, right._ncols).zero_matrix()

So the result is the immutable zero matrix. Compare:

sage: matrix.identity(RDF, 4) * matrix(RDF, 4, 0)
[]
sage: _.is_mutable()
False
sage: matrix.identity(QQ, 4) * matrix(QQ, 4, 0)
[]
sage: _.is_mutable()
True

I think the most consistent thing is to return an immutable matrix. So that is what I changed it to.


New commits:

214e9b4Return a mutable matrix for Matrix_double_dense even if one of the dimensions is 0.

comment:3 follow-up: Changed 8 months ago by chapoton

maybe we can also take the opportunity to fix the un-necessary subdivision ?

comment:4 in reply to: ↑ 1 Changed 8 months ago by gh-mwageringel

Replying to chapoton:

sage: A = matrix.identity(QQ, 4)                                             
sage: A._subdivisions
sage: A.subdivide(A.subdivisions())                                             
sage: A._subdivisions                                                           
( [ 0, 4 ], [ 0, 4 ] )

It looks like this is not a problem.

sage: A = matrix.identity(QQ, 4)
sage: A.subdivisions()
([], [])
sage: A.subdivide(A.subdivisions())
sage: A.subdivisions()
([], [])

I assume that A._subdivisions contains [0, 4] just because it makes it more conventient to work with, internally.

comment:5 Changed 8 months ago by git

  • Commit changed from 214e9b4bbced06cd0a09dc21f47f606a110f62e5 to 2fefdb156e00dcc589e3b48e7de17d8fd81aa223

Branch pushed to git repo; I updated commit sha1. New commits:

2fefdb131234: fix trailing whitespace

comment:6 in reply to: ↑ 2 Changed 8 months ago by gh-mwageringel

  • Reviewers set to Markus Wageringel
  • Status changed from needs_review to positive_review

Replying to tscrim:

I think the most consistent thing is to return an immutable matrix. So that is what I changed it to.

Thank you for the fix. I have removed some trailing whitespace. Other than that, this looks good to me, so I am setting this to positive.

comment:7 in reply to: ↑ 3 Changed 8 months ago by tscrim

Replying to chapoton:

maybe we can also take the opportunity to fix the un-necessary subdivision ?

I agree that this should be fixed as well. However, I think it is best as a separate ticket, which is now #31254.

comment:8 Changed 8 months ago by vbraun

  • Branch changed from public/matrix/double_zero_matrix_mult-31234 to 2fefdb156e00dcc589e3b48e7de17d8fd81aa223
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.