-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Matrix connected components decomposition #19045
Matrix connected components decomposition #19045
Conversation
✅ Hi, I am the SymPy bot (v158). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6. 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.
Update The release notes on the wiki have been updated. |
🟠Hi, I am the SymPy bot (v158). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
If these files were added/deleted on purpose, you can ignore this message. |
Codecov Report
@@ Coverage Diff @@
## master #19045 +/- ##
=============================================
+ Coverage 75.737% 75.787% +0.049%
=============================================
Files 648 649 +1
Lines 168798 168852 +54
Branches 39786 39798 +12
=============================================
+ Hits 127844 127969 +125
+ Misses 35387 35319 -68
+ Partials 5567 5564 -3 |
sympy/matrices/graph.py
Outdated
if M[i, i] is M.zero: | ||
E.append((i, i)) | ||
|
||
return strongly_connected_components((V, E)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be connected_components
in which case duplicate edges don't need to be added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With todok this can just be:
return connected_components((range(N), M.todok()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, the test fails in python3.5 probably because of dictionary ordering. Does connected_components
make unique results? If not, I'm thinking of sorting the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results returned from connected_components can be in any order. The strongly_connected_components function returns components in topological order which is also not unique.
What is the expected way to get the blocks from a block matrix. I see I can use args but I expected blocks to do it except:
There also seems to be a bug here:
I guess that should be |
I see sympy/sympy/matrices/expressions/blockmatrix.py Lines 306 to 312 in 08534bb
I think that the only thing api that came before to serve the purpose is |
This is looking good. I hope some linear algebra users will smile after seeing this. I'll leave you and Oscar to figure out when it is ready. |
The reason I ask about args is because looking at this the first thing that comes to my mind is how do I actually get the blocks if I want to use them for something. I think that the doc should explain how to use this. The purpose of getting the blocks is so that you can compute eigenvalues etc so you need the block as a matrix on its own. Putting it altogether in a blockmatrix as a decomposition is nice but in practice not what is wanted in a situation where you would want to use this. |
Looks good. |
References to other Issues or PRs
#16207
Brief description of what is fixed or changed
I see there were some needs of using
connected_components
.So I wrap two APIs for this first for finding connected components and second for finding a similar matrix which is block diagonal.
I would also wrap most of the new decomposition methods using the most economical structure possible (with block diagonal matrix and permutation matrix) because
But someone would find these stuff unintuitive or difficult to use since I don't think that matrix expressions API is more famous than
Matrix
.In the examples, I have just used
as_explicit
because it prints block diagonal matrix into some awkward forms.Other comments
Release Notes
connected_components
andconnected_components_decomposition
for matrix which decomposes a matrix into a block diagonal form.todok
function to find dictionary of keys format from any dense or sparse matrices.BlockDiagMatrix.get_diag_blocks
to provide an user API to get diagonal blocks from the matrix.