-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add (dense) Cholesky up- and downdates #14243
Conversation
7fa2116
to
e485029
Compare
|
|
How about calling it |
@ViralBShah Which of them? @tkelman One method of a generic function has to be the first. It's clear from the signature that is is a linear algebra method and it won't collide with any use outside linear algebra. However, it will probably not be the most widely used linear algebra function, so I'd be okay with removing the entries in |
If any packages are currently exporting something named |
I think it would only be a problem if they exported a function with the exact same signature, but did something different. Otherwise they could just import |
No way – Base cannot export such a generic name used in such a non-general way. Extending the function with unrelated functionality that happens to have the same name is also not ok. In order to have something like this exported from Base you have to be able to write some general description of what |
"Update" and "downdate" refer to fixed-rank additive or subtractive modifications of matrix factorizations - in principle applicable to any matrix factorization, not just Cholesky, and not necessarily rank 1. So it would be possible to treat "update" in the matrix factorization sense as "the" way to Maybe it's best to call them |
@StefanKarpinski I'm fine with not exporting it from Lately, I've been splitting up method documentation entries instead of having the larger entries covering several methods. I think it makes them more precise and easier to maintain. Is that the wrong direction? |
1adf699
to
f72cd24
Compare
""" | ||
update!(C::Cholesky, v::StridedVector) -> CC::Cholesky | ||
|
||
Update a Cholesky factorization `C` with the vector `v`. If `A = C[:U]'C[:U]` then `CC = cholfact(C[:U]'C[:U] _ v*v')` but the computation of `CC` only uses `O(n^2)` operations. The input factorization `C` is updated in place such that on exit `C == CC`. The vector `v` is destroyed during the computation. |
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.
No indent at the start of the line.
Please, please, add line breaks to docstrings. Please.
0b25019
to
b4794be
Compare
b4794be
to
6840a25
Compare
""" | ||
update!(C::Cholesky, v::StridedVector) -> CC::Cholesky | ||
|
||
Update a Cholesky factorization `C` with the vector `v`. If `A = C[:U]'C[:U]` then `CC = cholfact(C[:U]'C[:U] + v*v')` but the computation of `CC` only uses `O(n^2)` operations. The input factorization `C` is updated in place such that on exit `C == CC`. The vector `v` is destroyed during the computation. |
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.
We have discussed this before and you know we don't agree. It's not to annoy you, but I think it is wrong to use hard line break in prose. The section you link to was added because of long code lines and didn't consider doc strings. Using soft-wrapping in prose is in line with how |
Add (dense) Cholesky up- and downdates
It's inline documentation in source code. Not the same thing as prose. Unreadable and a pain to review and edit. Not just on github, plenty of local editors as well. You may think hard line breaks are wrong (would it actively harm anything other than your personal taste to add them?), but I'd guess that as many or more contributors think not adding them is wrong for readability. Aesthetic disagreements like this just cause everyone additional work and hassle, so we need to agree on a convention and stick to it. I'll shut up if we make that decision universally, but it's going to need more opinions than just yours behind it. |
As discussed in JuliaLang/LinearAlgebra.jl#6. Cholmod also provides this functionality in the sparse case so it should be easy to add that.
Only issue is that
update!
in theCHOLMOD
is used as method name for reusing a symbolic factorization. Theupdate!
methods in theCHOLMOD
module are not exported so we can rename them and I think we should. I'll propose to just overloadcholfact
andldlfact
with methods taking both a factorization and a matrix.