Opened 15 months ago

Closed 9 months ago

#29257 closed defect (fixed)

use solve_left for division operation of matrices

Reported by: gh-mwageringel Owned by:
Priority: major Milestone: sage-9.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:

Status badges

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 non-square 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() < 1e-14
True

Change History (12)

comment:1 Changed 15 months ago by gh-mwageringel

  • Authors set to Markus Wageringel
  • Branch set to u/gh-mwageringel/29257
  • Commit set to 828d67aa9d9732e340d879a2293e31a1378dce38
  • Dependencies set to #12406
  • Status changed from new to needs_review

Based on #12406.


New commits:

95e4157Fix minor typo.
8c41acfFix hidden bug in RDF/CDF matrix multiplication dead error-checking.
5844bbeChange RDF/CDF matrix multiplication to accept matrices for B.
fab2bcdImprove exceptions
bd6e4fcMerge tag '9.1.beta4' into #17405
7b07a2717405: fix solve_right and solve_left over RDF and CDF
0ccb65a12406: implement coercion for generic solve_right
a815ff912406: explicitly convert to AA in generalized_permutahedron
828d67a29257: use solve_left for __truediv__ operation of matrices and vectors

comment:2 Changed 15 months ago by git

  • Commit changed from 828d67aa9d9732e340d879a2293e31a1378dce38 to 859900e4fac586b00343342fb99abcccf52a01d3

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

859900e29257: use solve_left for __truediv__ operation of matrices and vectors

comment:3 Changed 15 months ago by gh-mwageringel

Now this also passes the tests with Python 2.

comment:4 Changed 14 months ago by git

  • Commit changed from 859900e4fac586b00343342fb99abcccf52a01d3 to be62f788c4047f0f8a04a398e16368aaba09fc04

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

828d9db12406: implement reviewer suggestions
6d0bbf612406: add a doctest to check consistency of coercion in solve_right
d751e78Trac # 12406: document that "check" means nothing over inexact rings.
71f0c28Trac #12406: absorb RDF/CDF solve_* into the superclass methods.
9aa2eb3Trac #12406: unbreak doctests for exact rings.
988e21512406: move rank checks to _solve_right_nonsingular_square
99c2a7e12406: test that `check` is ignored over inexact rings
c1178d912406: add NotFullRankError for _solve_right_nonsingular_square
93cea8512406: test that the solution is inexact
be62f7829257: use solve_left for __truediv__ operation of matrices and vectors

comment:5 Changed 14 months ago by gh-mwageringel

Rebased on top of #12406.

comment:6 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.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 9 months ago by git

  • Commit changed from be62f788c4047f0f8a04a398e16368aaba09fc04 to 653849a0ec393a672103560aae9cfc78d398e1d6

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

653849aMerge tag '9.2.beta8' into #29257

comment:8 Changed 9 months ago by gh-kliem

  • Reviewers set to Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

comment:9 Changed 9 months ago by gh-mwageringel

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 9 months ago by gh-kliem

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 9 months ago by gh-mwageringel

Ok, sounds good to me.

comment:12 Changed 9 months ago by vbraun

  • Branch changed from u/gh-mwageringel/29257 to 653849a0ec393a672103560aae9cfc78d398e1d6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.