Feature/issue 125 cholesky derivative#135
Conversation
memory usage for large matrices
|
drops the mic, quits On Thursday, July 30, 2015, Rob Trangucci notifications@github.com wrote:
|
|
Ha, just wait, it'll fail unit tests and/or cpplint soon. Rob
|
|
Rob's not quitting Stan, he's just changing day jobs :-) I do love those memory numbers. I'd also say Ben or Marcus should be reviewing this one,
|
No storage of double* L_ in ctor Eigen access using coeffRef instead of raw arrays Use info about row-major algo to eliminate an O(N^2) loop after O(N^3) loop
|
@syclik ? The error on travis says |
|
I'll take a look tonight. On Sunday, August 2, 2015, Rob Trangucci notifications@github.com wrote:
|
|
Thanks Daniel Rob On Sun, Aug 2, 2015 at 7:42 PM, Daniel Lee notifications@github.com wrote:
|
|
@bob-carpenter @syclik I think this is probably ready for code review whenever you're ready. Thanks guys. |
|
Can you get Marcus or Ben to review it? My matrix algebra's
|
|
@bgoodri Would you be able to take a look at this? @bob-carpenter I had @mbrubake look at my code, it sounded like he thought the code was up to snuff from a C++-optimization standpoint. That's actually why I wanted you to look at it too, because I thought you'd have a good perspective on memory allocation, etc. |
|
I had separately given Rob my feedback and suggestions on the vari implementation so I don't really have much to add there. One thing though which is missing is a gradient test which doesn't use the constrained transformation. The tricky thing about getting the (autodiff) gradients of something like Cholesky correct is how to properly handle the symmetry when propagating the derivatives. For instance, it's common for the gradient of the upper or lower triangular portion to be zero when it shouldn't be or to be off by a factor of 2. @rtrangucci can you add tests which don't use |
There was a problem hiding this comment.
The variables should be private if you're being picky.
|
with |
|
But the test actually survives the previous problem and aborts later on a double free. In gdb, |
|
Was there a version of this working that we can revert to for the
|
|
I tried to git bisect it and as usual, didn't make much headway due to On Fri, Aug 21, 2015 at 10:45 PM, Bob Carpenter notifications@github.com
|
|
I think we're just going to all have to stare at this until we figure out the memory problem. The which seems to suggest that which seems like an odd way of counting up to something that is too big. Then I changed the inner loop to continue only until which causes the test to pass so I am going to push that. |
|
This failing test in which is perhaps technically positive definite if it is considered symmetric. Anyway, we should set its elements to zero explicitly but on a different PR. |
|
@bgoodri Let me try committing a version of the code that doesn't respect the memory locality of the input and output matrices. I don't have gdb though, Ben; would you be able to run the -fsanitize=address flag? |
|
It looks fixed by 3f4f966 . Why are we going backwards? |
|
I misinterpreted; I thought there was still a problem after the commit. You can nuke my latest commit Rob
|
|
A test based on undefined behavior has no grounds to It isn't worth holding up this PR over, but we should eliminate or fix the test. |
|
Agreed about the test being wrong. And thanks for fixing.
|
|
I would say to git pull develop into the cholesky branch and then git revert 14815b3 Ben On Tue, Aug 25, 2015 at 12:22 AM, Rob Trangucci notifications@github.com
|
|
Cool, will do this afternoon so we can this merged. Rob
|
|
@bgoodri ok'ed this at the meeting today. |
…vative Fixes #125. Feature/issue 125 cholesky derivative
|
Is that ok that it's not the memory locality preserving commit?? I thought we wanted to revert to Ben's last commit and then merge... Rob
|
|
should I revert this pull request? On Tue, Aug 25, 2015 at 11:33 AM, Rob Trangucci notifications@github.com
|
|
Yes, you should revert the pull request, revert my last commit on the PR and then merge the branch into develop. Or I can do it later today. Either way. Rob
|
|
fuck. might have to revert the revert. On Tue, Aug 25, 2015 at 12:00 PM, Rob Trangucci notifications@github.com
|
|
I got it. On Tue, Aug 25, 2015 at 12:04 PM, Daniel Lee bearlee@alum.mit.edu wrote:
|
|
it's a new pull request: #156 |
6 participants
Summary:
Introduces var specialization of cholesky_decompose
Intended Effect:
Wall-time boost of about about 30% to 50% over currently implemented naive autodiff-through-Eigen version, memory use O(D^2) instead of O(D^3).
How to Verify:
./runTests.py test/unit/math/rev/mat/fun/cholesky_decompose_test.cpp
runs two gradient tests comparing results to finite diff.
Side Effects:
None.
Documentation:
Included in code.
Reviewer Suggestions:
Anyone