Add probability tests to check vectorized/scalar lpmfs/lpdfs are the same#2085
Add probability tests to check vectorized/scalar lpmfs/lpdfs are the same#2085
Conversation
… log cdf, and a log ccdf (previously was just testing f([a, b, c, d], Issue #1861)
…4.1 (tags/RELEASE_600/final)
t4c1
left a comment
t4c1
left a comment
There was a problem hiding this comment.
For now I just reviewed the first file. Others look similar and I have a number of questions. So I will review the rest after this makes sense to me.
Enabled first order forward mode autodiff tests on probability distributions (Issue #1861)
…into bugfix/issue-1861-2
|
@t4c1 ready for review again, though we might want to wait for tests to pass again since we're checking more things now that I enabled the |
|
@bbbales2 You have some failed tests here |
Replaced gradient checks (Issue #1861)
…into bugfix/issue-1861-2
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
t4c1
left a comment
t4c1
left a comment
There was a problem hiding this comment.
There were a number of changes since the last review, so I am again stopping at first file.
test/prob/test_fixture_ccdf_log.hpp
Outdated
| "gradients"; | ||
| for (size_t i = 0; i < single_gradients2.size(); ++i) | ||
| EXPECT_NEAR(single_gradients2[i], multiple_gradients2[i], 1e-7) | ||
| << "scalar and vectorized results should have the same first order " |
There was a problem hiding this comment.
| << "scalar and vectorized results should have the same first order " | |
| << "scalar and vectorized results should have the same second order " |
There was a problem hiding this comment.
While this simplifies the code, the error message loses the information which variable has wrong gradient.
There was a problem hiding this comment.
This error information wasn't printed anywhere anyway.
There was a problem hiding this comment.
Oh, right I added it in #2082, but it is not merged yet. Could you print it? That is very useful info for debugging. (since it is not in yet lets make this optional)
There was a problem hiding this comment.
Just a minor note: you might want to use expect_near_rel for inexact comparisons - we thought a bit about good logic for this type of comparisons and also it works out of the box for vector/matrix arguments (see e.g. #1656)
There was a problem hiding this comment.
@martinmodrak switched to expect_near_rel and life did get easier, thanks for rec.
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Green check green check green check! Man that feels good |
5 participants
This is the replacement for #1989
Fixes #1861 and #1978
This also adds similar tests to prevent problems with the log ccdf and cdf functions.
Release notes
Added more tests for vectorized probability functions
Checklist
Math issue Probability test framework didn't catch bug with vectorization #1861, vector distribution tests should test with vectors of not all the same values #1978
Copyright holder: (fill in copyright holder information)
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