Bugfix/implicit qr #3165
Bugfix/implicit qr #3165
Conversation
|
Need to add for some actions in the failed workflow to work for pull requests from users without write permissions to the repository. |
| @@ -158,7 +158,7 @@ void casadi_cvx_givens(T1 a, T1 b, T1* c, T1* s) { | |||
| // Golub & Van Loan Alg. 8.3.2 | |||
| template<typename T1> | |||
| void casadi_cvx_implicit_qr(casadi_int n, T1* t_diag, T1* t_off, T1* cs) { | |||
| T1 d, mu, to2, x, z, c, s, t1, t2, d0, d1, o0, o1, sd; | |||
| T1 d, mu, to2, x, z, c, s, t1, t2, d0, d1, o0, sd; | |||
| casadi_int i; | |||
There was a problem hiding this comment.
@victorfors A general comment: this code isn't readable. There should be comments on each line explaining every step of the algorithm
There was a problem hiding this comment.
Ok, I will add some comments in this function where it makes sense.
I've been going through some of the code in this file, would you want a pull request with added comments if I find some other places where I think they would help readability?
There was a problem hiding this comment.
My personal opinion is that all code should be properly commented or be removed from the repo. I'm not sure what the story is for this particular code however. I think pull requests for adding comments are always welcome.
There was a problem hiding this comment.
Added comments. Sorry for the merge noise in the commit history but I assume you can squash that if you merge.
At the final iteration in casadi_cvx_implicit_qr the code accessed the out of bounds element t_off[n-1]. t_off[i+1], x, and z are no longer updated in the final iteration (i=n-2).