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

Add (dense) Cholesky up- and downdates #14243

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Add (dense) Cholesky up- and downdates #14243

merged 1 commit into from
Dec 9, 2015

Conversation

andreasnoack
Copy link
Member

As discussed in #2929. Cholmod also provides this functionality in the sparse case so it should be easy to add that.

Only issue is that update! in the CHOLMOD is used as method name for reusing a symbolic factorization. The update! methods in the CHOLMOD module are not exported so we can rename them and I think we should. I'll propose to just overload cholfact and ldlfact with methods taking both a factorization and a matrix.

@andreasnoack andreasnoack force-pushed the anj/cholupdate branch 2 times, most recently from 7fa2116 to e485029 Compare December 3, 2015 01:45
@kshyatt kshyatt added the domain:linear algebra Linear algebra label Dec 3, 2015
@tkelman
Copy link
Contributor

tkelman commented Dec 3, 2015

cholfact! with separate destination (reused factorization) and source maybe?

@tkelman
Copy link
Contributor

tkelman commented Dec 3, 2015

update is a very generic name to be exporting from base, but this is a very specific linear algebra operation so I'm not sure it's worth claiming the namespace for yet. Maybe leave this as LinAlg.update at first?

@ViralBShah
Copy link
Member

How about calling it updatefactor?

@andreasnoack
Copy link
Member Author

@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 exports.jl.

@tkelman
Copy link
Contributor

tkelman commented Dec 3, 2015

If any packages are currently exporting something named update wouldn't adding the export to base cause issues?

@andreasnoack
Copy link
Member Author

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 update from Base. In JuliaStats we decided to define and export many names in StatsBase because that could be the most upstream package for most other statistics packages. Then two downstream packages that don't have a dependency on eachother can both import that name from StatsBase.

@StefanKarpinski
Copy link
Sponsor Member

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 means. This documentation is entirely specific to Cholesky, which is really not acceptable.

@jiahao
Copy link
Member

jiahao commented Dec 3, 2015

"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 update! a matrix factorization object.

Maybe it's best to call them LinAlg.update! and LinAlg.downdate!.

@andreasnoack
Copy link
Member Author

@StefanKarpinski I'm fine with not exporting it from Base or renaming it to rankUpdate or updateRank, but to avoid misunderstadings: updates like these are not specific to Cholesky. It's basically a rank update of the underlying matrix. As pointed out by @jiahao: eventually, we should also have rank update methods for non-factorized matrices, QR and eigen factorizations. It just happened that the Cholesky came first.

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?

@andreasnoack andreasnoack force-pushed the anj/cholupdate branch 2 times, most recently from 1adf699 to f72cd24 Compare December 7, 2015 04:09
"""
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.
Copy link
Contributor

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.

@andreasnoack andreasnoack force-pushed the anj/cholupdate branch 3 times, most recently from 0b25019 to b4794be Compare December 7, 2015 19:46
"""
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreasnoack
Copy link
Member Author

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 md files are handled now. It doesn't work for our doc strings and it doesn't on the phones, as you pointed out last time, so I've send a request to GitHub for a fix.

andreasnoack added a commit that referenced this pull request Dec 9, 2015
Add (dense) Cholesky up- and downdates
@andreasnoack andreasnoack merged commit 26457ce into master Dec 9, 2015
@andreasnoack andreasnoack deleted the anj/cholupdate branch December 9, 2015 01:06
@tkelman
Copy link
Contributor

tkelman commented Dec 9, 2015

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.

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.

6 participants