var matrix specializations for a-c unary functions in rev #2256
Conversation
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
|
THere are a few functions like this in #2254. I was stuck there on what to do with Does what you did here also work with I asked @andrjohns about this in the other thread but maybe you have already solved it (in which case don't worry about it @andrjohns !)
That should be testable now. I had to add these tests for |
|
Lemme test that. It should at least be close to working. All this does is overload each function so when apply scalar iterates over the elements it should just do the normal thing and differ to the other specializations until it hits varmat and then just do the varmat thing |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
stan/math/prim/fun/acos.hpp
Outdated
| template <typename Container, | ||
| require_not_container_st<std::is_arithmetic, Container>* = nullptr> | ||
| require_not_container_st<std::is_arithmetic, Container>* = nullptr, | ||
| require_not_var_matrix_t<Container>* = nullptr> | ||
| inline auto acos(const Container& x) { | ||
| return apply_scalar_unary<acos_fun, Container>::apply(x); | ||
| } |
There was a problem hiding this comment.
@andrjohns I'm having a hard time removing this require_not_var_matrix_t<Container> and I'd like to not have to put these everywhere. Right now this function works for any stan scalar or container that does not have an arithmetic scalar type. So it's kind of a mixed bag of types this takes in. Though specializations for each seems like overkill as well so I'm not really sure if we can make this look nicer
|
@bbbales2 fyi I removed var matrix from the container types. It feels like a var matrix is a single unit and we are pretty much always going to have specializations for var matrix. With the container checks we're normally checking whether it's an eigen vector so we can do some specialization or a std vector so we can recursivly go through it (or to map the std vector to an eigen matrix to use the Eigen specialization). So like I think with var matrix it makes sense to not have it count as a container. Does that make sense? |
|
But if its not a container, is it a scalar? I dont think that make sense. |
|
Yeah I'm not totally sure what it is either, but I'm also not using it like the other things we call containers. Popping it out seems fine with me. Seems like whenever we use I guess we could change |
…4.1 (tags/RELEASE_600/final)
…h into feature/varmat-a-to-c-unary
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@SteveBronder this ready to review? I want to just copy whatever you do here for the scalar functions in #2254 so if it's done that's convenient lol. |
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
|
@bbbales2 this should be good to go |
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
stan/math/prim/fun/abs.hpp
Outdated
| * @return absolute value of scalar | ||
| * @tparam Container type of container | ||
| * @param x argument | ||
| * @return Arc cosine of each variable in the container, in radians. |
stan/math/prim/fun/atan.hpp
Outdated
| @@ -39,8 +39,7 @@ struct atan_fun { | |||
| */ | |||
| template <typename Container, | |||
| require_not_container_st<std::is_arithmetic, Container>* = nullptr, | |||
| require_all_not_nonscalar_prim_or_rev_kernel_expression_t< | |||
| Container>* = nullptr> | |||
| require_not_var_matrix_t<Container>* = nullptr> | |||
stan/math/prim/fun/abs.hpp
Outdated
| * <code>std::abs(int)</code>. | ||
| * @tparam T type of variable | ||
| * @param x argument | ||
| * @return Arc cosine of variable in radians. |
stan/math/prim/fun/abs.hpp
Outdated
| * | ||
| * @tparam Container Type of x | ||
| * @param x argument | ||
| * @return Arc cosine of each variable in the container, in radians. |
| @@ -83,8 +83,9 @@ struct acosh_fun { | |||
| * @param x container | |||
| * @return Elementwise acosh of members of container. | |||
| */ | |||
| template <typename T, require_all_not_nonscalar_prim_or_rev_kernel_expression_t< | |||
| T>* = nullptr> | |||
| template < | |||
There was a problem hiding this comment.
I guess this is the other thing I'd expect:
template <typename Container,
require_container_st<std::is_arithmetic, Container>* = nullptr>
inline auto acosh(const Container& x) {
return apply_vector_unary<Container>::apply(
x, [](const auto& v) { return v.array().acosh(); });
}There was a problem hiding this comment.
Actually this can just be template <typename T, require_container_t<T>* = nullptr>
There was a problem hiding this comment.
There were some functions here that didn't have apply_vector_unary/Container implementations. I'm assuming it's because the Eigen array functions didn't exist for them to be written (cbrt, this, and others). I don't think cleanup is what we're going for in this pull really, so it's fine to leave it.
Edit: didn't exist at the time these functions were written but do exist now
There was a problem hiding this comment.
(But if you want to clean things, feel free, happy to review)
There was a problem hiding this comment.
Ups actually ignore that, this needs to be able to take in scalar types so that's why we can't use require_constainer_st
| template <typename F, typename T> | ||
| struct apply_scalar_unary<F, T, require_var_matrix_t<T>> { | ||
| /** | ||
| * Function return type, which is <code>var</code>. |
test/unit/math/mix/fun/acos_test.cpp
Outdated
| A << -2.2, -0.8, 0.5, 1 + std::numeric_limits<double>::epsilon(), 1.5, 3, 3.4, | ||
| 4; | ||
| expect_ad_matvar(f, A); | ||
| std::vector<Eigen::MatrixXd> A_vec; |
There was a problem hiding this comment.
The easier way to build these is with initializer lists. Something like:
std::vector<Eigen::MatrixXd> A_vec = { A, A, A };
| @@ -16,3 +16,18 @@ TEST(mathMixCore, atan2) { | |||
| stan::test::expect_ad(f, 1.2, 3.9); | |||
| stan::test::expect_ad(f, 0.5, 2.3); | |||
| } | |||
| /* Not sure how to test this so I'll come back to it. | |||
There was a problem hiding this comment.
This is only for unary stuff so another time
| using stan::math::asinh; | ||
| return asinh(x1); | ||
| }; | ||
| auto com_args = stan::test::internal::common_args(); |
There was a problem hiding this comment.
I think all these tests should be for varmat with vector, row vector and Matrix vector arguments.
How about:
expect_ad_common_vector_matvar(f);And it does:
expect_ad_vector_matvar(f, stan::math::to_vector(stan::test::internal::common_args()));
// not sure if `common_args` is std::vector or notAnd that function is something like:
expect_ad_vector_matvar(F& f, const auto& x) {
Eigen::VectorXd v = x;
Eigen::RowVectorXd r = x.transpose();
Eigen::MatrixXd m(x.size(), 2);
com_args_m.block(0, 0, com_args_v.size(), 1) = x;
com_args_m.block(0, 1, com_args_v.size(), 1) = x.reverse(); // reverse just to mix stuff up
expect_ad_matvar(f, v);
expect_ad_matvar(f, r);
expect_ad_matvar(f, m);
std::vector<Eigen::VectorXd> vv = { v, v };
std::vector<Eigen::RowVectorXd> rv = { r, r };
std::vector<Eigen::MatrixXd> mv = { m, m };
expect_ad_matvar(f, vv);
expect_ad_matvar(f, rv);
expect_ad_matvar(f, mv);
}Also does std::vector<std::vector<...>> work? Don't think it needs tested I'm just curious.
There was a problem hiding this comment.
Yeah I think it makes sense to test matrix vector row vector at least to make sure they compile. I'll write something like this up
…unary. Uses .adj() method instead of accessing .adj_ directly for vari
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
| template <typename F, typename T> | ||
| struct apply_scalar_unary<F, T, require_var_matrix_t<T>> { | ||
| /** | ||
| * Function return type, which is `var_value<plain_type_t<T>>`. |
There was a problem hiding this comment.
Not quite correct. I'm not sure exactly how to say this, but it's a var_value with a plain inner type?
I went with:
Function return type, which is a `var_value` with plain value type.
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Ty! Yes I just went through and removed the |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
This is just going through rev and adding var matrix functions for a bunch of functions.
Tests
expect_ad_matvartests added for all of the functions in here. Though I still need to write tests forstd::vector<var<matrix>>Side Effects
I wrote this by specializing a function for each rev version. But that means we have to write the
scalar_unaryversion likeThough there may be some other way to do this where instead we specialize
*_funAs it's written right now it's nice because it works with
std::vector<var<matrix>>. eod we need to specialize somewhere so I don't mind having it like we do now. We could also makevar<matrix>not a container type so then we can remove therequire_not_var_matrix_t<Container>Release notes
Adds arc trig functions for
var<matrix>along several other unary operatorsChecklist
Math issue How to add static matrix? #1805
Copyright holder: Steve Bronder
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