-
-
Notifications
You must be signed in to change notification settings - Fork 190
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 derivatives for mvn #2980
Add derivatives for mvn #2980
Conversation
This test now passes ./stan/math/prim/prob/multi_normal_lpdf.hpp:237:11: note: in instantiation of function template specialization 'Eigen::Matrix<double, -1, 1>::operator=<Eigen::Matrix<stan::math::var_value<double>, -1, 1>>' requested here
half = mdivide_left_ldlt(ldlt_Sigma, y_val_minus_mu_val); The offending code is at
ldlt_factor needs to be a double but it's passing a const stan::math::var_value<double> . This is beyond my current cpp skills.
|
…dev/math into multi-normal-derivatives-2
…dev/math into multi-normal-derivatives-2
@bgoodri this doesn't benchmark any faster than the current autodiff code. Does it make sense to add this for stability or should we just keep the current autodiffed code? |
I would say that if it is no slower, then having the analytical derivatives is worth it because it consumes less RAM. |
@SteveBronder this should be good for a review once the CI finishes |
@spinkney, just letting you know there are merge conflicts; @SteveBronder approved the PR! |
@syclik @SteveBronder fixed the merge conflict and updated to the develop branch. If this passes then it should be good to go! |
@serban-nicusor-toptal does CI need to be restarted? I've never seen this error before. |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins shenanigans, everything looks fine now! |
Our CI has a minor race condition where the exact following sequence of events can cause trouble
Step (3) will fail because it is still using the stanc3 binaries from before the merge in (2) There isn't a good way (that I know of) to avoid this, so just giving the pipeline a kick to restart when this does happen seems like the best thing to do |
Summary
This PR adds derivatives for the
multi_normal_lpdf
.Tests
Added the derivative tests in
test/unit/math/rev/prob/multi_normal_test.cpp
Side Effects
No
Release notes
Added hand-calculated derivatives for the multivariate normal lpdf.
Checklist
Copyright holder: Sean Pinkney
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