Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use solve_left for division operation of matrices #29257

Closed
mwageringel opened this issue Feb 28, 2020 · 20 comments
Closed

use solve_left for division operation of matrices #29257

mwageringel opened this issue Feb 28, 2020 · 20 comments

Comments

@mwageringel
Copy link

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

Depends on #12406

Component: linear algebra

Author: Markus Wageringel

Branch/Commit: 653849a

Reviewer: Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/29257

@mwageringel
Copy link
Author

Branch: u/gh-mwageringel/29257

@mwageringel
Copy link
Author

Author: Markus Wageringel

@mwageringel
Copy link
Author

Dependencies: #12406

@mwageringel
Copy link
Author

comment:1

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

@mwageringel
Copy link
Author

Commit: 828d67a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 28, 2020

Changed commit from 828d67a to 859900e

@mwageringel
Copy link
Author

comment:3

Now this also passes the tests with Python 2.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2020

Changed commit from 859900e to be62f78

@mwageringel
Copy link
Author

comment:5

Rebased on top of #12406.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2020

comment:6

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.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2020

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

653849aMerge tag '9.2.beta8' into #29257

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2020

Changed commit from be62f78 to 653849a

@kliem
Copy link
Contributor

kliem commented Aug 15, 2020

Reviewer: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Aug 15, 2020

comment:8

LGTM.

@mwageringel
Copy link
Author

comment:9

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.

@kliem
Copy link
Contributor

kliem commented Aug 15, 2020

comment:10

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

@mwageringel
Copy link
Author

comment:11

Ok, sounds good to me.

@vbraun
Copy link
Member

vbraun commented Aug 16, 2020

Changed branch from u/gh-mwageringel/29257 to 653849a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants