add plain and cholesky rank updates #4387
Conversation
| @@ -591,8 +591,8 @@ void CGMM::max_likelihood(SGMatrix<float64_t> alpha, float64_t min_cov) | |||
| switch (cov_type) | |||
| { | |||
| case FULL: | |||
| linalg::dger( | |||
| alpha.matrix[j * alpha.num_cols + i], v, v, cov_sum); | |||
| linalg::rank_update( | |||
There was a problem hiding this comment.
@iglesias the old dget routine was a desaster, I added a new one, pls double check
| auto y_martix = | ||
| SGVector<T>::convert_to_matrix(y, A.num_cols, 1, false); | ||
|
|
||
| auto temp_martix = SGMatrix<T>::matrix_multiply( |
There was a problem hiding this comment.
@iglesias it created a temp matrix, called a multiply method, and then added it.
The new one doesnt create a temp matrix. It only works for x=y, though can be easily extended and we didnt need x!=y in any calls of the dger
|
fixes #4385 and prepares ground for a linear regression with bias update |
|
Eigen3 can also do this, but it only updates the lower or upper part of a matrix. We might want to think about introducing types link symmetric matrices where only half of the memory is effectively used and the rest can be used a temp storage by algorithms, like eigen does |
|
@OXPHOS btw is there a reason why we don't define the methods in linalg using a type trait etc Could this replace the macros to define things for certain ptypes? |
|
I can not recall any reason for not using `enable_if`. We actually used it
for one scenario. I can look into it soon. Just finished my defense ;D
…On Tue, Jul 31, 2018 at 7:30 AM Heiko Strathmann ***@***.***> wrote:
@OXPHOS <https://github.com/OXPHOS> btw is there a reason why we don't
define the methods in linalg using a type trait
template <typename T, typename T2 = typename std::enable_if_t<std::is_arithmetic<T>::value>>
template <typename T, typename T2 = typename std::enable_if_t<std::is_floating_point<T>::value>>
template <typename T, typename T2 = typename std::enable_if_t<std::is_integral<T>::value>>
etc
Could this replace the macros to define things for certain ptypes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4387 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIaEMJ-AQkwbm1wnkl4qKOaW4ss3OMalks5uMD-2gaJpZM4VoEXW>
.
|
|
wow congrats! :) |
| T update; | ||
| for (auto j : range(A.num_cols)) | ||
| { | ||
| x = b[j]; |
There was a problem hiding this comment.
Out of curiosity, having the local x is better than just using b[j] all over the method? Same question for update.
| } | ||
|
|
||
| /** | ||
| * Updates a matrix \f$A\f$ with a rank one term in-place,If A = LL^T |
There was a problem hiding this comment.
Minor: I think for this method the the A = LL^T can be left out.
| * Updates the Cholesky factorization \f$A = L L^{*}\f$ with a rank one | ||
| * term in-place. | ||
| * If A = LL^T before the update, then after it | ||
| * LL^{*} = A + alpha * b b^{*} |
There was a problem hiding this comment.
Quite nitpicking: perhaps a different name for the L as they are not the same as the ones above. In wikipedia they use tilde analogously for A. It is completely subjective I think, whether we want the doc to be more math correct, or just reflect what happens in the code. I am happy either way :-)
No description provided.