Fix cholesky_decompose not updating adjoints correct (2.26)
#2365
Conversation
bbbales2
changed the title
cholesky_decompose not updating adjoints correct (2.26)
|
Will this test run correctly for opencl when opencl is turned on? I guess, did opencl have this problem? |
…4.1 (tags/RELEASE_600/final)
|
Based on https://github.com/stan-dev/math/blob/develop/stan/math/opencl/rev/cholesky_decompose.hpp#L72 I would say no? But I am going to push the same test to the OpenCL variant as well. |
| @@ -87,7 +89,7 @@ inline auto cholesky_lambda(T1& L_A, T2& L, T3& A) { | |||
| using Eigen::Lower; | |||
| using Eigen::StrictlyUpper; | |||
| using Eigen::Upper; | |||
| Eigen::MatrixXd L_adj(L.rows(), L.cols()); | |||
| Eigen::MatrixXd L_adj = Eigen::MatrixXd::Zero(L.rows(), L.cols()); | |||
There was a problem hiding this comment.
Oh interesting, was the issue here that the upper triangular part of the matrix was not being set?
There was a problem hiding this comment.
Actually I meant to leave a comment there. That one change I just did cause it looked scary and was too lazy to check the Eigen docs or carefully read the code.
There was a problem hiding this comment.
That one can def. be reverted but I just saw it and got nervous.
|
Looks like it failed on the upstream? https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/downstream_hotfix/7/pipeline/ |
|
Yeah, I dont get that error "You are not currently on a branch.". @serban-nicusor-toptal will know for sure :) |
|
I'm on it, checking to see what's wrong. |
|
Thanks! Seems like the issue on the Stan repo was fixed but now the same one popped up in CmdStan. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Hey it looks all fixed now. |
|
@bbbales2 merge? |
|
Can I press the button? |
|
How about we align on discourse about 2.26.1 ? |
Fix `cholesky_decompose` not updating adjoints correct (2.26)
Summary
Fixes #2364 . The problem was
cholesky_decomposenot updating the input adjoints correctly (but doing it in a way that it would pass generic autodiff tests).Tests
Added one!
Release notes
cholesky_decomposenot propagating derivatives correctlyChecklist
Math issue
cholesky_decomposenot propagating derivatives correctly in 2.26 #2364Copyright holder: Columbia University
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested