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
mat: rewrite extractions and solves as methods on factorising types #113
Conversation
7c5422d
to
bcffb4b
Compare
8a18df8
to
5358abf
Compare
Here's a meta comment, just to make sure I'm thinking about this correctly in the code review. I would love to figure out how to make higher-level libraries be efficient with their use of matrix types. Let's say (in a future gonum), we have I believe moving the Solve routines into I also think if we do this properly, this opens the door for really nice interoperability with sparse matrices. For instance, our current linear programming implementation forces using dense matrices, which means it'll never compete with "real" lp solvers. But, we could create some interface that's like
This interface can now be implemented by Cholesky (for Dense LPs, or structured sparse LPs), and then by some outside/future package that provides those routines for a sparse matrix and Cholesky decomposition. This is the opportunity I think we can provide with a potentially huge benefit. I'd like to see if we're on the same page. |
Moving the methods onto the factorising types I don't think changes this (first and second part), but can be used for the lp part.
My main motivation is just ease of legibility.
Note that there's another sweep that needs to go through that allows many of these methods to take nil and allocate returning the result. Also, I think Cholesky.To may want to be called ToSymmetric. What do you think?
Finally, with the addition of the band types, we get diagonals for free, so that can just be type switched in the appropriate places.
|
mat/cholesky.go
Outdated
// A = U^T * U. | ||
func (t *TriDense) UFromCholesky(chol *Cholesky) { | ||
func (chol *Cholesky) UTo(dst *TriDense) *TriDense { |
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.
Please unify the name of the receiver.
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.
Thanks. Done.
c6be534
to
fa212df
Compare
I have not made errorable methods (inverse and solve) able to allocate a destination and return it. This can be done later if people want it. |
@btracey Please take a look.
Fixes #101.
Fixes #102.