-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Support extracting factors from cholfact(::SparseMatrixCSC) #10402
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is not a good idea as you also anticipated in your comment. The problem
LPx=bhas to be solvedYy=bfirst and thenPx=y, i.e. apply the inverse permutation to the solution. The operationA[pinv,pinv]is probably quite expensive for a sparse matrix, but destroying the triangular structure will require a new factorization before you can solve your problem.I can see two solutions. Either
F[:LP]returns aPivotedCholeskyFactoror we only support extractingF[:L]andF[:p]and it would be the responsibility of the user to apply the permutations after the solve. I prefer the last solution for simplicity for the users as well as the maintainers.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.
But note this function isn't about extracting individual factors: this is just so that
sparse(cholfact(A)) == A.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 ones for extracting components are just a few lines down from here, and indeed they follow the prescription you suggest.)
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.
Sorry for not reading carefully enough. I can see that
FactorComponentacts like the suggestedPivotedCholeskyFactor. I'm still in doubt if it is a good idea to combineLandPin one type. It is already on the todo to make a permutation matrix type and we could support extractingPfrom a non-pivoted dense Cholesky where it would then be almost a noop inPx=y. Then code could still be generic for dense and sparse input.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.
I'm simply glad you're reading it at all, no apologies needed.
To be clear, a
FactorComponentis any type of factor: it can represent just:Lor:D(meaning, it also represents single factors) or:PtLor:LDor ...Rather than mushing them together into a single type, I think it would be fine to have separated factors using an expanded type hierarchy:
L = cholfact[:L]; P = cholfact[:P]. But I do wonder whether ready-made combinations are still desirable, in much the same way that aCholeskyis a nice convenience that guards against dumb mistakes likex = L\(L'\b)(instead ofL'\(L\b)as it should be). Or stated another way, what advantages do you see that ourcholfacttypes offer that's different fromL, p = chol(A)?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
cholfacttypes don't actually require constructing the mathematically-valid representation of the factors, they can keep the internal data represented however the library chooses, right? So you don't have to constructLunless requested, which is potentially an expensive step that isn't needed for solving a system of equations.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.
Yes,
F[:PtL]returns an opaque object.