Generic var templates for operators and std::iterator_trait var/fvar specialization #1525

SteveBronder · 2019-12-18T06:15:27Z

Summary

This pulls out / cleans up some of the stuff in the complex branch. Namely

Pulls out and tidies up the templates that are added to the var operators
Adds std::iterator_traits specializations for var and fvar
Adds cmath member functions to the stan::math namespace for autodiff types

Tests

@bob-carpenter do need additional tests for the iterators?

Side Effects

std will have `std::iterator_trait specializations for var and fvar

Checklist

Math issue complex numbers with vars #123
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
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

…rator templates use requires_ to take in more generic value types

…xp1~20180509124008.99 (branches/release_50)

…stable/2017-11-14)

…ath into cleanup/generic-templates-var

stan-buildbot · 2019-12-19T00:03:41Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.97)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.01)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 0.98)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 0.98)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.01)
(performance.compilation, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 0.95)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.05)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.01)
Result: 0.99815799169
Commit hash: ec6345a

SteveBronder · 2019-12-19T18:19:18Z

@bob-carpenter ready for review!

bob-carpenter · 2019-12-20T16:18:47Z

@SteveBronder Should I really be reviewing this given that I wrote some of it?

SteveBronder · 2019-12-20T19:28:33Z

Good point! Would anyone else mind taking a look at this?

bob-carpenter · 2019-12-23T22:40:52Z

OK. I'll just go ahead and review today or tomorrow---it will have two of us looking at all the code. This is going to take some reviewing and I have to head out soon, so I can get to it tomorrow.

mcol

I've read through this mainly because I happened to see how you dealt with std::isinf(), which hit me in #1545 (and I had worked around it in not such a nice way). It turned out that I think I've learned a lot of new things, so it was time well spent.

As you'll see, most of comments concern the ordering of typenames that doesn't match the ordering of function arguments. Of course that's not a big deal, so I leave it up to you what do about that.

I totally skipped stan/math/rev/core/std_iterator_traits.hpp because I can't even pretend to know what use it has, and also stan/math/rev/scal/fun/pow.hpp because I haven't cracked std::forward yet. So you should get a second pair of more experienced eyes on this! :)

stan/math/rev/core/operator_greater_than_or_equal.hpp

stan/math/rev/core/operator_less_than.hpp

stan/math/rev/core/operator_less_than_or_equal.hpp

stan/math/rev/core/operator_logical_and.hpp

stan/math/prim/meta/require_generics.hpp

stan/math/rev/core/cmath.hpp

stan/math/rev/mat/functor/algebra_solver_fp.hpp

stan/math/rev/core/operator_addition.hpp

SteveBronder · 2019-12-23T23:57:54Z

I've read through this mainly because I happened to see how you dealt with std::isinf(), which hit me in #1545 (and I had worked around it in not such a nice way). It turned out that I think I've learned a lot of new things, so it was time well spent.

Glad to hear and thanks for looking this over!

As you'll see, most of comments concern the ordering of typenames that doesn't match the ordering of function arguments. Of course that's not a big deal, so I leave it up to you what do about that.

I think that's v reasonable to fix up

I totally skipped stan/math/rev/core/std_iterator_traits.hpp because I can't even pretend to know what use it has, and also stan/math/rev/scal/fun/pow.hpp because I haven't cracked std::forward yet. So you should get a second pair of more experienced eyes on this! :)

That's fair, after snooping around online ~~the link below makes me think they are much harder to implement than I originally thought. I'll probably have to rework these~~

~~https://github.com/tzlaine/stl_interfaces~~

Edit: Eh that seems like more than we need

…gs/RELEASE_500/final)

SteveBronder · 2019-12-24T01:48:14Z

OK. I'll just go ahead and review today or tomorrow---it will have two of us looking at all the code. This is going to take some reviewing and I have to head out soon, so I can get to it tomorrow.

Worst case we had 3 sets of eyes on it now (thanks again @mcol!) so I think that's fine

stan-buildbot · 2019-12-24T07:46:10Z

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.98)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 0.96)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.0)
(performance.compilation, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.02)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 0.98)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.0)
Result: 0.99921351037
Commit hash: 36777ab

mcol · 2019-12-24T09:07:39Z

stan/math/rev/core/precomputed_gradients.hpp

@@ -80,16 +83,22 @@ class precomputed_gradients_vari : public vari {
 * specified value, vector of operands, and vector of partial
 * derivatives of value with respect to the operands.
 *
+ * @tparam Arith An arithmetic type
+ * @tparam VecVar A vector of vars
+ * @tparam VecArith A vector of


bob-carpenter

My requests mainly fall into three broad categories:

break each function into its own file and document everything
replace comparisons and logical operators with single definition in prim using value_of_rec
explain why we have Var template parameters combined with require_var_t<V> rather than just var in all the overloads

I think this would've been easier broken into two PRs, one with the updated metaprograms and one with updated operators, but I'm OK keeping it as is given that the review's in progress.

bob-carpenter · 2019-12-23T22:42:46Z

stan/math/rev/core/cmath.hpp

@@ -0,0 +1,119 @@
+#ifndef STAN_MATH_REV_CORE_CMATH_HPP


This is only a few things from <cmath>---the rest of the functions defined in that header follow the Stan convention of being broken into their own files. I don't think we want to stray from that convention here. Sorry for all the extra work---it's one of the reasons I was dreading breaking this all out into tests and definitions.

looking at cmath it looks like all these are in the cmath header? For clarification you want signbit, isinf, isfinite, isnan, copysign, and isnormal to be in their own header files?

bob-carpenter · 2019-12-23T22:48:09Z

stan/math/rev/core/cmath.hpp

+template <typename T, require_autodiff_t<T>...>
+inline bool signbit(T&& v) {
+  using std::signbit;
+  return signbit(v.val());


Can we write this with value_of_rec(v) and make it generic across our autodiff types? Or havit recurse with v.val() if both forward-mode and reverse-mode support .val(). Either way, it can go in prim as it's not mentioning any autodiff types itself.

Yeah since fvar and var have .val() we can move this to prim. Should we also have each of these for arithmetic types or just rely on the cmath impl for those (in case someone calls stan::math::signbit(a) where a is double)?

If we wanted to be v smart we could break these up to have signatures for

Arithmetic types

AD types with an arithmetic return from .val()

AD types with heavier types in .val() (i.e. complex<>/fvar<var>/fvar<fvar<var>>)

Then 1 and 2 would pass by copy while 3 would be able to forward along / pass a reference to .val()

bob-carpenter · 2019-12-24T22:43:58Z

stan/math/prim/meta/require_generics.hpp

@@ -473,24 +473,22 @@ using require_any_not_fvar_t
    = require_any_not_t<is_fvar<std::decay_t<Types>>...>;

 template <typename T>
-using require_var_or_fvar_t = require_t<is_var_or_fvar<T>>;
+using require_autodiff_t = require_t<is_autodiff<T>>;


[comment]
These are all super useful. Thanks for adding.

bob-carpenter · 2019-12-24T22:48:06Z

stan/math/prim/meta/require_generics.hpp


 template <typename T>
-using require_not_var_or_fvar_t = require_not_t<is_var_or_fvar<T>>;
+using require_not_autodiff_t = require_not_t<is_autodiff<T>>;


I'd strongly prefer

removing the require_ prefix because that's implying a use, not a behavior

replace _t with _v because it denotes a boolean value, not a type; this follows the behavior of is_arithmetic_v; enable_if_t ends in _t because it denotes a type (void by default), not a value.

For example, that'd be not_autodiff_v<T> for true if T is not an autodiff type, and usage would be, for example enable_if_t<not_autodiff_v<T>, U>.

removing the require_ prefix because that's implying a use, not a behavior

Can you clarify? I'm not sure i understand how the require part of the name is implying a use over a behaviour. The intent with the require_ in the name is because all those require_*s are trying to emulate a legacy version of C++20 concepts. It's for if you want a universal reference in the function signature for all the weird value types and const combinations. In terms of use/behavior I think my answer is that I think about C++ concepts like compile time contracts (which feels like a behaviour, though idk what that word means in context here)

replace _t with _v because it denotes a boolean value, not a type; this follows the behavior of is_arithmetic_v; enable_if_t ends in _t because it denotes a type (void by default), not a value.
For example, that'd be not_autodiff_v for true if T is not an autodiff type, and usage would be, for example enable_if_t<not_autodiff_v, U>.

The require_*_t metaprogramming stuff is essentially a convoluted / compact way to write std::enable_if_t<...> i.e. they also return void.

You can think of require_autodiff_t as an alias for enable_if_t<not_autodiff_v<T>, U>.

Also idt we can use C++ compile time constexpr objects with c++1y (anything _v like how std does) since the windows compiler we require compatibility with for Rtools has a bug for those

I misunderstood and thought this was a boolean. Now that I see what it's doing, I still think it needs to be renamed. How about enable_if_autodiff_t to enable if it's an autodiff type? That follows the standard library naming conventions more closely than require_autodiff_t.

We can then use disable_if_autodiff_t instead of enable_if_not_autodiff_t.

We can build similar ones for primitives, enable_if_arithmetic_t and disable_if_arithmetic_t.

bob-carpenter · 2019-12-24T22:48:15Z

stan/math/prim/meta/require_generics.hpp


 template <typename... Types>
-using require_all_var_or_fvar_t = require_all_t<is_var_or_fvar<Types>...>;
+using require_all_autodiff_t = require_all_t<is_autodiff<Types>...>;


Should these all have doc or are they so close to the ones without _v that they're obvious? cppreference actually doesn't doc these and they doc everything else meticulously.

I've been throwing this back and forth with myself. I've leaned towards not doc since aliases are inlined in the docs you can see the full definition

http://mc-stan.org/math/d0/dfb/group__require__base__types.html#gac55eae5c1fc2f38a96336b5f0bc94449

bob-carpenter · 2019-12-26T19:10:33Z

stan/math/rev/core/operator_unary_negative.hpp

 * @param a Argument variable.
 * @return Negation of variable.
 */
-inline var operator-(const var& a) {
+template <typename Var, require_var_t<Var>...>
+inline var operator-(Var&& a) {
  return var(new internal::neg_vari(a.vi_));


[optional as it wasn't in your code]
Just wanted to throw this out for the future. When we don't return refs, we can just use the braces constructor:

return { new internal::neg_vari(a.vi_) };

bob-carpenter · 2019-12-26T19:11:26Z

stan/math/rev/core/operator_unary_not.hpp

 * @param[in] x argument
 * @return negation of argument value
 */
-inline bool operator!(const var& x) { return !x.val(); }
+template <typename Var, require_var_t<Var>...>
+inline bool operator!(Var&& x) {


Because there are no derivatives, this can also be done as a single primitive template using value_of_rec.

Yep! I think with all these it would be good to do them as a seperate PR else this is going to blast off into like a 800-1000 line change PR

bob-carpenter · 2019-12-26T19:12:20Z

stan/math/rev/core/std_isinf.hpp

@@ -13,7 +13,8 @@ namespace std {
 * @param a Argument.
 * @return 1 if argument is infinite and 0 otherwise.
 */
-inline int isinf(const stan::math::var& a) {
+template <typename Var, stan::require_var_t<Var>...>
+inline bool isinf(Var&& a) {


This can also be made fully generic in prim.

bob-carpenter · 2019-12-26T19:14:14Z

stan/math/rev/mat/fun/gp_periodic_cov.hpp

@@ -257,7 +260,7 @@ class gp_periodic_cov_vari<T_x, double, T_l, T_p> : public vari {
        sin_2_dist_[pos] = sin(2.0 * pi_div_p * dist);
        sin_dist_sq_[pos] = sin_dist_sq;
        cov_lower_[pos] = new vari(
-            sigma_sq_d_ * std::exp(sin_dist_sq * neg_two_inv_l_sq), false);


I'm OK weither way with this, but if everything's guaranteed to be int or double, there's no reason not to directly qualify the function.

It feels like there's a reason i did this but I can't remember. I'll change it back and if something breaks we'll know what's up

bob-carpenter · 2019-12-26T19:16:24Z

stan/math/rev/scal/fun/pow.hpp

  }
-  return var(new internal::pow_vd_vari(base.vi_, exponent));
+  return var(
+      new internal::pow_vd_vari(base.vi_, static_cast<double>(exponent)));


Even though you're allowing exponent to be int now, I don't think you need the static cast to double if the only constructor available for pow_vd_vari does it for you.

…ces for return types

SteveBronder · 2020-01-24T05:22:53Z

Hmm, that's a really weird error I'll look into what's happening here tmrw

SteveBronder · 2020-01-24T05:55:07Z

Actually the only way I know how to fix this

./test/test-models/good/optimization/rosenbrock.hpp:177:73: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
             lp_accum__.add(-((pow((1 - x), 2) + (100 * pow((y - pow(x, 2)), 2)))));

Is to make the var pow() use a require so that it get's kicked out of the choices during SFINAE. This collision happens because the right hand side is an int instead of a double&. Is that cool? If we do it for one should we go back to doing that for all of them?

…gin' into cleanup/generic-templates-var

…ath into cleanup/generic-templates-var

…stable/2017-11-14)

SteveBronder · 2020-01-25T19:44:09Z

Actually Bob, kind of a wild take but should we pass all these vars by value? They are 8 bytes and like a double can just fit in a word**

** on 64 bit systems which I think is our main target anyway

SteveBronder · 2020-01-25T19:53:19Z

Uh, passing by value seems very good

Running performance-tests-cmdstan with

./compare-git-hashes.sh "stat_comp_benchmarks -j4 --runs 1" develop develop develop 993ceaa8629c59bfe1cebab746cdc50a7e894a4d

+ ./comparePerformance.py develop_performance.csv performance.csv markdown
---RESULTS---
| Name | Old Result | New Result | Ratio | Performance change( 1 - new / old ) |
| ------------- |------------- | ------------- | ------------- | ------------- |
| stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan | 3.22 | 3.21 | 1.0 | 0.44% faster |
| stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan | 0.01 | 0.01 | 1.11 | 9.7% faster |
| stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan | 0.06 | 0.06 | 1.0 | -0.18% slower |
| stat_comp_benchmarks/benchmarks/arK/arK.stan | 1.38 | 1.35 | 1.02 | 1.85% faster |
| stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan | 4.95 | 4.85 | 1.02 | 2.1% faster |
| performance.compilation | 73.0 | 66.67 | 1.1 | 8.68% faster |
| stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan | 6.86 | 6.93 | 0.99 | -1.08% slower |
| stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan | 15.27 | 15.66 | 0.98 | -2.55% slower |
| stat_comp_benchmarks/benchmarks/sir/sir.stan | 76.34 | 85.63 | 0.89 | -12.16% slower |
| stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan | 0.23 | 0.24 | 0.98 | -2.49% slower |
| stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan | 2.19 | 2.1 | 1.04 | 3.85% faster |
| stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan | 0.15 | 0.12 | 1.22 | 17.96% faster |
| stat_comp_benchmarks/benchmarks/arma/arma.stan | 0.53 | 0.52 | 1.02 | 2.13% faster |
| stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan | 0.03 | 0.03 | 1.08 | 7.67% faster |
| stat_comp_benchmarks/benchmarks/garch/garch.stan | 0.5 | 0.39 | 1.27 | 21.37% faster |
Mean result: 1.04750942953

bob-carpenter · 2020-01-26T21:47:17Z

We should be able to pass var by value. I assume 1.05 means it's faster? What's this testing?

SteveBronder · 2020-01-29T04:37:43Z

This is just running the basic cmdstan performance tests that get kicked off with every PR, but locally on my home computer

…up/generic-templates-var

bob-carpenter · 2020-01-29T21:02:38Z

I brought this up to date with develop. I think the last failure upstream was some kind of CI failure, not a problem with this PR.

…up/generic-templates-var

bob-carpenter · 2020-01-31T22:10:40Z

@SteveBronder : There's now an ambiguity in pow(double, int) surfacing in this PR:

./test/test-models/good/optimization/rosenbrock.hpp:177:31: error: call to 'pow' is ambiguous
            lp_accum__.add(-((pow((1 - x), 2) + (100 * pow((y - pow(x, 2)), 2)))));
                              ^~~
...
/usr/include/x86_64-linux-gnu/bits/mathcalls.h:153:17: note: candidate function
__MATHCALL_VEC (pow,, (_Mdouble_ __x, _Mdouble_ __y));
                ^
lib/stan_math/stan/math/rev/fun/pow.hpp:125:12: note: candidate function [with Arith = int, $1 = <>]
inline var pow(const var& base, Arith exponent) {
           ^
lib/stan_math/stan/math/rev/fun/pow.hpp:160:12: note: candidate function [with Arith = double, $1 = <>]
inline var pow(Arith base, const var& exponent) {
           ^
lib/stan_math/stan/math/rev/fun/pow.hpp:108:12: note: candidate function
inline var pow(const var& base, const var& exponent) {
           ^

mathcalls.h is coming in from the standard library math header.

All of these require one int to double promotion, hence the ambiguity. I'm not sure why it's not matching overload (7) mentioned in this doc:

https://en.cppreference.com/w/cpp/numeric/math/pow

It seems to be on my machine because I'm not getting the ambiguity.

The ambiguity arose in this PR because the pow function now matches the int or double with no promotion.

Would it be possible to replace the var with a template parameter that is required to be a var? Then it wouldn't match an int.

We might also consider adding our own pow(arithmetic, arithmetic) with templates. I'm not sure what kind of havoc that'd wreak with proper implementations of std::pow() overload (7) or with other implementations.

bob-carpenter · 2020-01-31T22:11:46Z

If you don't have time for this, do you mind if I cherry pick the bits I need for std::complex so that I can start staging that?

bob-carpenter · 2020-01-31T22:41:19Z

I tried the fully templating var approach. At least it doesn't seem to mess up math.

bob-carpenter · 2020-02-01T16:21:10Z

The problem now is that the test server is running out of memory. @serban-nicusor-catena ---should we just restart the tests or does something else need to happen?

g++  -std=c++1y -m64 -D_REENTRANT -Wall -Wno-unused-function -Wno-uninitialized -Wno-unused-but-set-variable -Wno-unused-variable -Wno-sign-compare -Wno-unused-local-typedefs      -I lib/stan_math/lib/tbb_2019_U8/include -O3 -I src -I . -I lib/stan_math/ -I lib/stan_math/lib/eigen_3.3.3 -I lib/stan_math/lib/boost_1.72.0 -I lib/stan_math/lib/sundials_5.1.0/include -I lib/stan_math/lib/gtest_1.8.1/include -I lib/stan_math/lib/gtest_1.8.1   -D_USE_MATH_DEFINES  -DBOOST_DISABLE_ASSERTS       -c src/test/unit/lang/parser/assignment_statement_test.cpp -o test/unit/lang/parser/assignment_statement_test.o

cc1plus.exe: out of memory allocating 2097112 bytes

cc1plus.exe: out of memory allocating 1442192 bytes
make/tests:13: recipe for target 'test/unit/lang/generator/generate_idxs_test.o' failed
mingw32-make: *** [test/unit/lang/generator/generate_idxs_test.o] Error 1
mingw32-make: *** Waiting for unfinished jobs....
make/tests:13: recipe for target 'test/unit/lang/generator/generate_cpp_test.o' failed
mingw32-make: *** [test/unit/lang/generator/generate_cpp_test.o] Error 1

rok-cesnovar · 2020-02-01T16:26:31Z

Argh, Nic did apply this fix https://www.intel.com/content/www/us/en/programmable/support/support-resources/knowledge-base/embedded/2016/cc1plus-exe--out-of-memory-allocating-65536-bytes.html but it seems to not do the trick.

It only occurs occasionally on the recently added Windows machine (running in our lab in Ljubljana). Let me know @serban-nicusor-toptal if I can do something you cant remotely. Will also buy some additional RAM.

I restarted the upstream tests https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1525/30/pipeline
If you only restart a stage Github doesnt show the yellow dot but if it will pass it will go green.

serban-nicusor-toptal · 2020-02-01T16:51:10Z

Thanks for the help here @rok-cesnovar
Ram and swap are more than enough I think so it may be a software configuration which doesn't let it allocate more maybe ... I'll look more into it tonight.
Edit: I think it's now fixed. Let me know if it happens again, thanks.

bob-carpenter · 2020-02-02T02:23:10Z

Yatta! The fully-templated version works. This is useful as I'll be able to do this elsewhere I've been having trouble with unwanted promotion of primitives to var.

@SteveBronder or @rok-cesnovar : would one of you review my changes to pow() here---nothing else has changed since it's been reviewed. I also added tests for instantiating pow.

I approved the changes I was waiting for.

SteveBronder · 2020-02-02T18:52:29Z

Sorry for the delay I'll take a look at this today

SteveBronder

lgtm with one small comment

SteveBronder · 2020-02-02T22:33:11Z

stan/math/rev/fun/pow.hpp

-inline var pow(const var& base, Arith exponent) {
+template <typename Var, typename Arith, require_var_t<Var>...,
+          require_arithmetic_t<Arith>...>
+inline var pow(const Var& base, Arith exponent) {


I'm fine with merging as is. Should we make a separate issue to have all these be passed by value?

Yes, that should definitely go in a separate issue, along with performance evaluations.

SteveBronder and others added 3 commits December 18, 2019 00:32

Adds std iterator_traits specialization for fvar and var. The var ope…

8309363

…rator templates use requires_ to take in more generic value types

cleanup the template names

bd9b20c

[Jenkins] auto-formatting by clang-format version 5.0.2-svn328729-1~e…

f8f2e3a

…xp1~20180509124008.99 (branches/release_50)

SteveBronder requested a review from bob-carpenter December 18, 2019 06:17

SteveBronder and others added 8 commits December 18, 2019 01:25

cleanup the unary var templates

8806331

cleanup the unary var templates

a6b4d81

[Jenkins] auto-formatting by clang-format version 5.0.2-svn328729-1~e…

3868acf

…xp1~20180509124008.99 (branches/release_50)

replace var_or_fvar_t with autodiff_t in require_generics tests

5e07ef9

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

119a7e7

…stable/2017-11-14)

remove template from ostream for var

1348dda

Merge branch 'cleanup/generic-templates-var' of github.com:stan-dev/m…

858e1f2

…ath into cleanup/generic-templates-var

fix order of members in kinsoldata

ec6345a

revert beta-binomial changes

4300497

mcol reviewed Dec 23, 2019

View reviewed changes

SteveBronder and others added 3 commits December 23, 2019 20:16

Fixup template order for core var impls

fd35443

Merge commit 'cb961050fd2bb013cc658eb8e3eafaa6bdf1cf38' into HEAD

73181b4

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

36777ab

…gs/RELEASE_500/final)

mcol reviewed Dec 24, 2019

View reviewed changes

mcol mentioned this pull request Dec 25, 2019

remove remaining uses of boost functions that are in std since C++11 #1545

Merged

5 tasks

bob-carpenter requested changes Dec 26, 2019

View reviewed changes

SteveBronder and others added 2 commits December 30, 2019 14:29

Moves cmath var and fvar stuff over to prim and uses initlializer bra…

ad62f4f

…ces for return types

Merge commit '65aec14f5caea8b9e38a7475afcd4a6681648497' into HEAD

fd2a4a8

fix isnan tests to call right fun

9eebaa7

SteveBronder and others added 5 commits January 25, 2020 13:42

testing pass by value for var ops instead of by reference

993ceaa

Merge branch 'cleanup/pass-var-by-value', remote-tracking branch 'ori…

cd60021

…gin' into cleanup/generic-templates-var

Merge branch 'cleanup/generic-templates-var' of github.com:stan-dev/m…

ced2df7

…ath into cleanup/generic-templates-var

Merge commit 'c0d2265f842a1b2df04855fac49d87e9962aa878' into HEAD

8d460ac

[Jenkins] auto-formatting by clang-format version 6.0.0 (tags/google/…

2795719

…stable/2017-11-14)

Merge branch 'develop' of https://github.com/stan-dev/math into clean…

99a916b

…up/generic-templates-var

Merge branch 'develop' of https://github.com/stan-dev/math into clean…

84f5b26

…up/generic-templates-var

templated var overloads for pow

06ad089

bob-carpenter approved these changes Feb 2, 2020

View reviewed changes

SteveBronder commented Feb 2, 2020

View reviewed changes

bob-carpenter merged commit f7e18c9 into develop Feb 3, 2020

bob-carpenter deleted the cleanup/generic-templates-var branch February 3, 2020 05:31

SteveBronder mentioned this pull request Apr 16, 2020

Stan Math 3.2 release #1826

Closed

bbbales2 mentioned this pull request Apr 20, 2020

Stanc3 release for Cmdstan 2.23 stan-dev/stanc3#498

Closed

Generic var templates for operators and std::iterator_trait var/fvar specialization #1525

Generic var templates for operators and std::iterator_trait var/fvar specialization #1525

Conversation

SteveBronder commented Dec 18, 2019 • edited Loading

Summary

Tests

Side Effects

Checklist

stan-buildbot commented Dec 19, 2019

SteveBronder commented Dec 19, 2019

bob-carpenter commented Dec 20, 2019

SteveBronder commented Dec 20, 2019

bob-carpenter commented Dec 23, 2019 • edited Loading

mcol left a comment

Choose a reason for hiding this comment

SteveBronder commented Dec 23, 2019 • edited Loading

SteveBronder commented Dec 24, 2019

stan-buildbot commented Dec 24, 2019

Choose a reason for hiding this comment

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bob-carpenter Dec 24, 2019 • edited by SteveBronder Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder commented Jan 24, 2020

SteveBronder commented Jan 24, 2020

SteveBronder commented Jan 25, 2020 • edited Loading

SteveBronder commented Jan 25, 2020

bob-carpenter commented Jan 26, 2020 via email

SteveBronder commented Jan 29, 2020

bob-carpenter commented Jan 29, 2020

bob-carpenter commented Jan 31, 2020

bob-carpenter commented Jan 31, 2020

bob-carpenter commented Jan 31, 2020

bob-carpenter commented Feb 1, 2020

rok-cesnovar commented Feb 1, 2020

serban-nicusor-toptal commented Feb 1, 2020 • edited Loading

bob-carpenter commented Feb 2, 2020

SteveBronder commented Feb 2, 2020

SteveBronder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder commented Dec 18, 2019 •

edited

Loading

bob-carpenter commented Dec 23, 2019 •

edited

Loading

SteveBronder commented Dec 23, 2019 •

edited

Loading

SteveBronder Dec 30, 2019 •

edited

Loading

bob-carpenter Dec 24, 2019 •

edited by SteveBronder

Loading

SteveBronder commented Jan 25, 2020 •

edited

Loading

serban-nicusor-toptal commented Feb 1, 2020 •

edited

Loading