Feature/lb constrain matvar varmat tests #2382
Conversation
… with std::vector arguments (Issue #1805)
…o feature/lb_constrain-matvar_varmat_tests
| } | ||
|
|
||
| // matrix[], matrix | ||
| { |
There was a problem hiding this comment.
Why are these in brackets like this?
There was a problem hiding this comment.
I was just marking off tests with arguments in Stan syntax so I could keep them straight in my head. We can delete these comments, of course. I just left them cause it reminds me what the code is doing.
| if(!any_varmat) { | ||
| SUCCEED(); // If no varmats are created, skip this test | ||
| return; | ||
| } |
There was a problem hiding this comment.
When would this happen?
There was a problem hiding this comment.
If a function was signature vector, real and these tests tried to check the matvar, varmat version of the function (which it will), then the matvar, matvar and matvar, varmat both use the C++ types Eigen::Matrix<var, -1, 1>, var so there's no point testing it.
It's important to actually not test this too, cause the return type will be matvar if there are not varmat arguments.
Three different notations for argument types in one sentence! Nice!
| auto A_mv_tuple = std::make_tuple(convert_to_matvar<Types>(x)...); | ||
| auto A_vm_tuple = std::make_tuple(convert_to_varmat<Types>(x)...); |
There was a problem hiding this comment.
Would a better name be make_matvar/varmat_compatible? Convert makes it sound like it's always going to transform to something else
| * @param x not used, only types are used. | ||
| */ | ||
| template <typename ReturnType, typename Type, require_matrix_t<Type>* = nullptr> | ||
| void check_return_type(const std::vector<ReturnType>& ret, |
There was a problem hiding this comment.
Why did this get deleted?
There was a problem hiding this comment.
Replaced by
math/test/unit/math/test_ad_matvar.hpp
Line 426 in ab230e6
Summary
@SteveBronder I was gonna do this directly on the lb_constrain-matvar branch but it ended up being big enough to be separate.
This:
std::vector<Eigen::Matrix<T, R, C>>types. This does not supportstd::vector<std::vector<...>>recursive types.lb_freeis failing to compile now though. I'm sending this back to you to implement. Merge this in the otherlb_constrainbranch when you're happy.Tests
I had to update the matvar test of tests quite a bit to make sure they were triggering in the right place
Release notes
Replace this text with a short note on what will change if this pull request is merged in which case this will be included in the release notes.
Checklist
Math issue Allow for
std::vectorarguments toexpect_ad_matvar#2333Copyright 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