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

Sparse triangular and diagonal solve bug #17254

Merged
merged 2 commits into from
Aug 5, 2019

Conversation

sylee957
Copy link
Member

@sylee957 sylee957 commented Jul 25, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

from sympy import *

m1 = SparseMatrix.diag([1, 2, 3])
m2 = Matrix([[1, 2], [3, 4], [5, 6]])
m1.diagonal_solve(m2)

Dense diagonal solve doesn't have this bug, but sparse diagonal solve is only solving the first column and giving the colunm vector.
Looks like sparse solver is using more inferiorly overridden version.

I've also found same bug for upper and lower triangular solve

Other comments

Release Notes

  • matrices
    • Fixed SparseMatrix.diagonal_solve only solving the first column of the RHS matrix.
    • Fixed SparseMatrix.upper_triangular_solve and SparseMatrix.lower_triangular_solve only solving the first column of the RHS matrix.
    • Fixed SparseMatrix.upper_triangular_solve and SparseMatrix.lower_triangular_solve not working if RHS is an immutable matrix.

@sympy-bot
Copy link

sympy-bot commented Jul 25, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • matrices
    • Fixed SparseMatrix.diagonal_solve only solving the first column of the RHS matrix. (#17254 by @sylee957)

    • Fixed SparseMatrix.upper_triangular_solve and SparseMatrix.lower_triangular_solve only solving the first column of the RHS matrix. (#17254 by @sylee957)

    • Fixed SparseMatrix.upper_triangular_solve and SparseMatrix.lower_triangular_solve not working if RHS is an immutable matrix. (#17254 by @sylee957)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

```python
from sympy import *

m1 = SparseMatrix.diag([1, 2, 3])
m2 = Matrix([[1, 2], [3, 4], [5, 6]])
m1.diagonal_solve(m2)
```
Dense diagonal solve doesn't have this bug, but sparse diagonal solve is only solving the first column and giving the colunm vector.
Looks like sparse solver is using more inferiorly overridden version. 

I've also found same bug for upper and lower triangular solve

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- matrices
  - Fixed `SparseMatrix.diagonal_solve` only solving the first column of the RHS matrix.
  - Fixed `SparseMatrix.upper_triangular_solve` and `SparseMatrix.lower_triangular_solve` only solving the first column of the RHS matrix.
  - Fixed `SparseMatrix.upper_triangular_solve` and `SparseMatrix.lower_triangular_solve`  not working if RHS is an immutable matrix.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

Copy link

@chankruze chankruze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me !

@sylee957
Copy link
Member Author

sylee957 commented Jul 25, 2019

I see there are some other issues with upper_triangular_solve and lower_triangular_solve

m1 = SparseMatrix.diag([1, 2, 3])
m2 = Matrix([[1, 2], [3, 4], [5, 6]])
m1.upper_triangular_solve(m2)

It gives a completely wrong result like Matrix([[1, 2], [3/2, 4], [5/3, 6]])
It may be quick to fix.

@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #17254 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##            master    #17254      +/-   ##
============================================
+ Coverage   74.548%   74.568%   +0.02%     
============================================
  Files          623       623              
  Lines       161785    161784       -1     
  Branches     37966     37967       +1     
============================================
+ Hits        120608    120640      +32     
+ Misses       35827     35793      -34     
- Partials      5350      5351       +1

@sylee957 sylee957 changed the title Sparse diagonal solve bug Sparse triangular and diagonal solve bug Jul 25, 2019
@sylee957 sylee957 merged commit c60fec5 into sympy:master Aug 5, 2019
@sylee957 sylee957 deleted the fix_diagonal_solve_sparse branch September 2, 2019 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants