Opened 2 years ago
Closed 18 months ago
#29257 closed defect (fixed)
use solve_left for division operation of matrices
Reported by:  ghmwageringel  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Markus Wageringel  Reviewers:  Jonathan Kliem 
Report Upstream:  N/A  Work issues:  
Branch:  653849a (Commits, GitHub, GitLab)  Commit:  653849a0ec393a672103560aae9cfc78d398e1d6 
Dependencies:  #12406  Stopgaps: 
Description
This ticket improves the __truediv__
division operation of matrices and vectors by implementing it using solve_left
(the _backslash_
operator is already implemented using solve_right
).
Currently, __truediv__
only works for square matrices with the same parent. The implementation naively computes the inverse of a matrix, which should be avoided, especially over inexact rings.
With the changes of this ticket (and #12406), the operation works over differing rings and even with nonsquare matrices.
sage: a = matrix(ZZ, [[1, 2], [0, 3]]) sage: b = matrix(ZZ, 3, 2, range(6)) sage: b / a == b * ~a True sage: a = matrix(ZZ, [[1, 2], [0, 3], [1, 5]]) sage: (b / a) * a == b True sage: b = vector(RDF, [1.5, 2.5]) sage: (b / a * a  b).norm() < 1e14 True
Change History (12)
comment:1 Changed 2 years ago by
 Branch set to u/ghmwageringel/29257
 Commit set to 828d67aa9d9732e340d879a2293e31a1378dce38
 Dependencies set to #12406
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Commit changed from 828d67aa9d9732e340d879a2293e31a1378dce38 to 859900e4fac586b00343342fb99abcccf52a01d3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
859900e  29257: use solve_left for __truediv__ operation of matrices and vectors

comment:3 Changed 2 years ago by
Now this also passes the tests with Python 2.
comment:4 Changed 22 months ago by
 Commit changed from 859900e4fac586b00343342fb99abcccf52a01d3 to be62f788c4047f0f8a04a398e16368aaba09fc04
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
828d9db  12406: implement reviewer suggestions

6d0bbf6  12406: add a doctest to check consistency of coercion in solve_right

d751e78  Trac # 12406: document that "check" means nothing over inexact rings.

71f0c28  Trac #12406: absorb RDF/CDF solve_* into the superclass methods.

9aa2eb3  Trac #12406: unbreak doctests for exact rings.

988e215  12406: move rank checks to _solve_right_nonsingular_square

99c2a7e  12406: test that `check` is ignored over inexact rings

c1178d9  12406: add NotFullRankError for _solve_right_nonsingular_square

93cea85  12406: test that the solution is inexact

be62f78  29257: use solve_left for __truediv__ operation of matrices and vectors

comment:5 Changed 22 months ago by
Rebased on top of #12406.
comment:6 Changed 22 months ago by
 Milestone changed from sage9.1 to sage9.2
Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.
comment:7 Changed 18 months ago by
 Commit changed from be62f788c4047f0f8a04a398e16368aaba09fc04 to 653849a0ec393a672103560aae9cfc78d398e1d6
Branch pushed to git repo; I updated commit sha1. New commits:
653849a  Merge tag '9.2.beta8' into #29257

comment:8 Changed 18 months ago by
 Reviewers set to Jonathan Kliem
 Status changed from needs_review to positive_review
LGTM.
comment:9 Changed 18 months ago by
Thank you. There was a related discussion in #29226 which has not been crosslinked here yet. IMHO, this is still an improvement over the current behavior, but it is okay if you would like to reconsider the review.
comment:10 Changed 18 months ago by
I see. Of course we can remove division of matrices, but even our documentation https://doc.sagemath.org/html/en/tutorial/tour_linalg.html advertises using the backslash.
For the time being it's an improvement. Even if we want to get rid of it eventually, at this point we could raise a deprecation error more easily and say, "use solve left/right instead".
comment:11 Changed 18 months ago by
Ok, sounds good to me.
comment:12 Changed 18 months ago by
 Branch changed from u/ghmwageringel/29257 to 653849a0ec393a672103560aae9cfc78d398e1d6
 Resolution set to fixed
 Status changed from positive_review to closed
Based on #12406.
New commits:
Fix minor typo.
Fix hidden bug in RDF/CDF matrix multiplication dead errorchecking.
Change RDF/CDF matrix multiplication to accept matrices for B.
Improve exceptions
Merge tag '9.1.beta4' into #17405
17405: fix solve_right and solve_left over RDF and CDF
12406: implement coercion for generic solve_right
12406: explicitly convert to AA in generalized_permutahedron
29257: use solve_left for __truediv__ operation of matrices and vectors