Feature/issue 202 vectorize all #399

rayleigh · 2016-09-28T15:22:36Z

Submisison Checklist

[x ] Run unit tests: ./runTests.py test/unit
[ x] Run cpplint: make cpplint
[ x] Declare copyright holder and open-source license: see below

Summary:

This pull request is to add the headers for the functions vectorized in #202 to stan/math/prim/mat.hpp so that the vectorized functions can be exposed in Stan.

Intended Effect:

None to users. Internally, I added the headers to stan/math/prim/mat.hpp.

How to Verify:

Check that vectorized functions are added correctly and tests still pass.

Side Effects:

None.

Documentation:

None needed.

Reviewer Suggestions:

Copyright and Licensing

Per Michigan's copyright laws, Rayleigh Lei is the copyright holder.
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

By submitting this pull request, the copyright holder is agreeing to 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/)

rayleigh · 2016-09-28T15:25:46Z

I couldn't add lgamma or fabs because there are other template functions using those functions and unit tests fail when adding them to stan/math/prim/scal/fun/mat.hpp. Specifically, stan/math/prim/scal/fun/lbeta.hpp is templated and uses lgamma whereas stan/math/prim/scal/fun/grad_2F1.hpp is templated and uses fabs.

bob-carpenter · 2016-09-28T15:41:28Z

I'm working on this in #370 starting by eliminating math.h branch and then will add vectorization for what's left in #347. #370 already has a pull request and #347 has already added a few more functions that weren't in the original request, but I'm going to stage #347 after #370.

bob-carpenter · 2016-09-28T15:41:55Z

I wasn't sure how this pull request was supposed to help with only defining a mat.hpp header file.

syclik · 2016-09-28T16:47:53Z

I'm confused too. Do those headers all exist?

rayleigh · 2016-09-28T20:49:20Z

I'll change the description, but I was adding the functions already vectorized in #202. In that huge pull request, I forgot to add those functions to stan/math/prim/mat.hpp. As a result, I think only log and exp can be vectorized in Stan currently even though we have more vectorized functions in stan/math.

rayleigh · 2016-09-29T15:12:19Z

I was looking at this and realized that I hadn't added the headers to stan/math/prim/mat.hpp in the last pull request. My other comment was about functions that are vectorized in stan/math, but couldn't be added to stan/math/prim/mat.hpp without the unit tests failing. I did some investigation and my comment is my guess at why I couldn't add them without the tests failing.

rayleigh · 2016-10-04T15:59:57Z

When I tried adding the function signature test for the matrix form of abs in Stan, I get make error 252. However, the test is a copy of the exp matrix test that passes, only with abs replacing the exp calls. Based on this, I think the cause of the make error is because I hadn't added the appropriate headers to stan/math/prim/mat.hpp in the last vectorize-all pull request in stan-math. Hence, I created this pull request to fix that problem by adding the headers to stan/math/prim/mat.hpp.

Indeed, I just tried adding the function signature test for the matrix form of log in Stan because the header file is in stan/math/prim/mat.hpp. The log matrix test is a copy of the exp matrix test, again with log replacing the exp calls. The log test did not error out.

So, if I can get this pull request in, minus the two functions I have had trouble adding to stan/math/prim/mat.hpp, I should be able to push the vectorized functions up to Stan.

bob-carpenter · 2016-10-04T16:21:51Z

When my pull request for remving math.h is merged (should be soon), then we will have stan::math::round we can use instead of ::round from math.h, which isn't portable to Windows compilers.
Why are you removing includes like cov_exp_quad.hpp?

bob-carpenter · 2016-10-04T16:22:34Z

Actually, that's already merged, so you should update and use stan::math::round.

rayleigh · 2016-10-04T16:32:13Z

I'll switch it. I'm using ::round because the integration tests were failing due to std::round last week.
No functions were removed. I essentially took out all functions in stan/math/prim/mat/fun/, added vectorized function headers, sorted all functions alphabetically using bash's sort, and then put it back in stan/math/prim/mat.hpp. As a result, some existing functions got moved around.

…re/issue-202-vectorize-all

bob-carpenter · 2016-10-04T16:39:44Z

(1) Thanks.

You only get std::round in C++11. But it's built into some
of the g++ and clang++ implementations. Overall, the libraries
are a mess before C++11 but get standardized properly in C++11.

(2) I see. That's OK. They should go in the alphabetical
order you rearranged them into.

Bob

On Oct 4, 2016, at 12:32 PM, rayleigh [email protected] wrote:

I'll switch it. I'm using ::round because the integration tests were failing due to std::round last week.

No functions were removed. I essentially took out all functions in stan/math/prim/mat/fun/, added vectorized function headers, sorted all functions alphabetically using bash's sort, and then put it back in stan/math/prim/mat.hpp. As a result, some existing functions got moved around.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

rayleigh · 2016-10-07T19:29:30Z

I updated round to use stan::math::round. Is this good to merge? Or, do I need to also add lgamma and fabs to stan/math/prim/mat.hpp as well? Or, is there something else as well?

bob-carpenter · 2016-10-09T04:29:53Z

I'm OK with this merging---I can fix the merge conflict it'll
cause on my existing branch that vectorizes the remaining
functions. I'm stuck on translation unit tests failing.

But what I don't see is how you can get away with putting
the code in stan/math/prim/mat.hpp---I wound up needing to
put it in stan/math/mix/mat.hpp to make sure it went after
all the core fwd and rev stuff so it could get instantiated
properly.

Bob

rayleigh · 2016-10-11T18:19:47Z

I'm curious what you mean by that.

If I include the vectorized version of lgamma.hpp in prim/math/mat.hpp and run test/unit/math/mix/scal/meta/child_type_test.cpp, I get the following error messages:

In file included from test/unit/math/mix/scal/meta/child_type_test.cpp:3:
In file included from ./test/unit/math/prim/scal/fun/promote_type_test_util.hpp:4:
In file included from ./stan/math/prim/mat.hpp:48:
In file included from ./stan/math/prim/mat/fun/abs.hpp:4:
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:39:41: error: implicit
      instantiation of undefined template
      'Eigen::internal::traits<unsigned int>'
      typedef typename Eigen::internal::traits<T>::Scalar scalar_t;
                                        ^
./stan/math/prim/mat/fun/lgamma.hpp:33:21: note: in instantiation of template
      class 'stan::math::apply_scalar_unary<stan::math::lgamma_fun, unsigned
      int>' requested here
    inline typename apply_scalar_unary<lgamma_fun, T>::return_t
                    ^
./stan/math/prim/mat/prob/lkj_corr_log.hpp:65:52: note: while substituting
      deduced template arguments into function template 'lgamma'
      [with T = unsigned int]
            - 0.25 * (Km1 * Km1) * LOG_TWO - Km1 * lgamma((K + 1) / 2);
                                                   ^
lib/eigen_3.2.9/Eigen/src/Core/util/ForwardDeclarations.h:17:29: note: template
      is declared here
template<typename T> struct traits;
                            ^
In file included from test/unit/math/mix/scal/meta/child_type_test.cpp:3:
In file included from ./test/unit/math/prim/scal/fun/promote_type_test_util.hpp:4:
In file included from ./stan/math/prim/mat.hpp:48:
In file included from ./stan/math/prim/mat/fun/abs.hpp:4:
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:45:39: error: type
      'unsigned int' cannot be used prior to '::' because it has no members
      typedef Eigen::Matrix<scalar_t, T::RowsAtCompileTime,
                                      ^
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:39:41: error: implicit
      instantiation of undefined template
      'Eigen::internal::traits<stan::math::var>'
      typedef typename Eigen::internal::traits<T>::Scalar scalar_t;
                                        ^
./stan/math/prim/mat/fun/lgamma.hpp:33:21: note: in instantiation of template
      class 'stan::math::apply_scalar_unary<stan::math::lgamma_fun,
      stan::math::var>' requested here
    inline typename apply_scalar_unary<lgamma_fun, T>::return_t
                    ^
./stan/math/prim/scal/fun/lbeta.hpp:61:14: note: while substituting deduced
      template arguments into function template 'lgamma' [with T =
      stan::math::var]
      return lgamma(a)
             ^
./stan/math/rev/scal/fun/grad_inc_beta.hpp:37:20: note: in instantiation of
      function template specialization 'stan::math::lbeta<stan::math::var,
      stan::math::var>' requested here
      var c3 = exp(lbeta(a, b)) * inc_beta(a, b, z);
                   ^
lib/eigen_3.2.9/Eigen/src/Core/util/ForwardDeclarations.h:17:29: note: template
      is declared here
template<typename T> struct traits;
                            ^
In file included from test/unit/math/mix/scal/meta/child_type_test.cpp:3:
In file included from ./test/unit/math/prim/scal/fun/promote_type_test_util.hpp:4:
In file included from ./stan/math/prim/mat.hpp:48:
In file included from ./stan/math/prim/mat/fun/abs.hpp:4:
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:45:42: error: no member
      named 'RowsAtCompileTime' in 'stan::math::var'
      typedef Eigen::Matrix<scalar_t, T::RowsAtCompileTime,
                                      ~~~^
4 errors generated.
make: *** [test/unit/math/mix/scal/meta/child_type_test.o] Error 1
make test/unit/math/mix/scal/meta/child_type_test failed
exit now (10/11/16 13:34:36 EDT)

If I go into stan/math/prim/mat/prob/lkj_corr_log.hpp and add using stan::math::lgamma to the function do_lkj_constant, the only function that uses lgamma, I get the following error message from running the same test:

In file included from test/unit/math/mix/scal/meta/child_type_test.cpp:3:
In file included from ./test/unit/math/prim/scal/fun/promote_type_test_util.hpp:4:
In file included from ./stan/math/prim/mat.hpp:48:
In file included from ./stan/math/prim/mat/fun/abs.hpp:4:
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:39:41: error: implicit
      instantiation of undefined template
      'Eigen::internal::traits<stan::math::var>'
      typedef typename Eigen::internal::traits<T>::Scalar scalar_t;
                                        ^
./stan/math/prim/mat/fun/lgamma.hpp:33:21: note: in instantiation of template
      class 'stan::math::apply_scalar_unary<stan::math::lgamma_fun,
      stan::math::var>' requested here
    inline typename apply_scalar_unary<lgamma_fun, T>::return_t
                    ^
./stan/math/prim/scal/fun/lbeta.hpp:61:14: note: while substituting deduced
      template arguments into function template 'lgamma' [with T =
      stan::math::var]
      return lgamma(a)
             ^
./stan/math/rev/scal/fun/grad_inc_beta.hpp:37:20: note: in instantiation of
      function template specialization 'stan::math::lbeta<stan::math::var,
      stan::math::var>' requested here
      var c3 = exp(lbeta(a, b)) * inc_beta(a, b, z);
                   ^
lib/eigen_3.2.9/Eigen/src/Core/util/ForwardDeclarations.h:17:29: note: template
      is declared here
template<typename T> struct traits;
                            ^
In file included from test/unit/math/mix/scal/meta/child_type_test.cpp:3:
In file included from ./test/unit/math/prim/scal/fun/promote_type_test_util.hpp:4:
In file included from ./stan/math/prim/mat.hpp:48:
In file included from ./stan/math/prim/mat/fun/abs.hpp:4:
./stan/math/prim/mat/vectorize/apply_scalar_unary.hpp:45:42: error: no member
      named 'RowsAtCompileTime' in 'stan::math::var'
      typedef Eigen::Matrix<scalar_t, T::RowsAtCompileTime,
                                      ~~~^
2 errors generated.
make: *** [test/unit/math/mix/scal/meta/child_type_test.o] Error 1
make test/unit/math/mix/scal/meta/child_type_test failed
exit now (10/11/16 13:36:48 EDT)

If I look at stan/math/prim/scal/fun/lbeta.hpp, I see that the function is templated. My guess is that it's pulling in the unary vectorize code out of order.

bob-carpenter · 2016-10-11T18:30:46Z

The fundamental problems are:

Bringing in a template definition before all
the classes needed to instantiate it. That's why I couldn't
figure out how to include in prim/mat.hpp. But I see
what you did and am going to try following that pattern right now.
Ambiguities due to not having a most specific
implementation. For example, providing an int
argument to a function defined only for double and
var --- int can be promoted to either one. That's why I've
added explicit int definitions where necessary.

I have the basic functions all sorted in a pull request that's
just awaiting Daniel's help for the multiple translation units.
And I vectorized the rest of the functions in a pull request waiting
on the first pull request.

I'd suggest just working on a pull request to get the rest of
the functions you wrote vectorized in Stan and by the time you're
done, I should have the math library sorted.

syclik · 2016-10-11T18:35:27Z

Yup. I need to work on that multiple translation unit thing.

On Oct 11, 2016, at 2:30 PM, Bob Carpenter [email protected] wrote:

The fundamental problems are:

Bringing in a template definition before all
the classes needed to instantiate it. That's why I couldn't
figure out how to include in prim/mat.hpp. But I see
what you did and am going to try following that pattern right now.

Ambiguities due to not having a most specific
implementation. For example, providing an int
argument to a function defined only for double and
var --- int can be promoted to either one. That's why I've
added explicit int definitions where necessary.

I have the basic functions all sorted in a pull request that's
just awaiting Daniel's help for the multiple translation units.
And I vectorized the rest of the functions in a pull request waiting
on the first pull request.

I'd suggest just working on a pull request to get the rest of
the functions you wrote vectorized in Stan and by the time you're
done, I should have the math library sorted.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

rayleigh · 2016-10-11T19:20:19Z

I see. Thanks for letting me know.

I'd suggest just working on a pull request to get the rest of the functions you wrote vectorized in Stan and by the time you're done, I should have the math library sorted.

That sounds good. Could I get this pull request merged then? I currently get a make error 252 when I try to vectorize in Stan the functions that are vectorized in stan/math. This pull request is to add the functions to stan/math/prim/mat.hpp with the exception of lgamma and fabs for reasons we've discussed above. Doing so will allow me to vectorize them in Stan because I've only been able to vectorize in Stan log and exp, functions that are in stan/math/prim/mat.hpp.

bob-carpenter

Looks good to me.

Rayleigh L added 3 commits September 27, 2016 15:34

Updated prim mat.hpp to include vectorized functions

158c4e4

Updated to exclude fabs and lgamma because of errors

31e9708

Updated to not error out

32dec76

Updated to round use ::round and not std::round

0d7f0af

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

d730b14

…re/issue-202-vectorize-all

Updated round to use stan::math::round

e5f7a3a

bob-carpenter added the code reviewed label Oct 13, 2016

bob-carpenter approved these changes Oct 13, 2016

View reviewed changes

bob-carpenter merged commit 1efa66c into develop Oct 13, 2016

rayleigh mentioned this pull request Oct 29, 2016

Remove unnecessary using statements from vectorized functions #426

Closed

syclik modified the milestone: v2.13.0 Nov 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 202 vectorize all #399

Feature/issue 202 vectorize all #399

rayleigh commented Sep 28, 2016 •

edited

Loading

rayleigh commented Sep 28, 2016

bob-carpenter commented Sep 28, 2016

bob-carpenter commented Sep 28, 2016

syclik commented Sep 28, 2016

rayleigh commented Sep 28, 2016 •

edited

Loading

rayleigh commented Sep 29, 2016

rayleigh commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

rayleigh commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

rayleigh commented Oct 7, 2016

bob-carpenter commented Oct 9, 2016

rayleigh commented Oct 11, 2016 •

edited

Loading

bob-carpenter commented Oct 11, 2016

syclik commented Oct 11, 2016

rayleigh commented Oct 11, 2016 •

edited

Loading

bob-carpenter left a comment

Feature/issue 202 vectorize all #399

Feature/issue 202 vectorize all #399

Conversation

rayleigh commented Sep 28, 2016 • edited Loading

Submisison Checklist

Summary:

Intended Effect:

How to Verify:

Side Effects:

Documentation:

Reviewer Suggestions:

Copyright and Licensing

rayleigh commented Sep 28, 2016

bob-carpenter commented Sep 28, 2016

bob-carpenter commented Sep 28, 2016

syclik commented Sep 28, 2016

rayleigh commented Sep 28, 2016 • edited Loading

rayleigh commented Sep 29, 2016

rayleigh commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

rayleigh commented Oct 4, 2016

bob-carpenter commented Oct 4, 2016

rayleigh commented Oct 7, 2016

bob-carpenter commented Oct 9, 2016

rayleigh commented Oct 11, 2016 • edited Loading

bob-carpenter commented Oct 11, 2016

syclik commented Oct 11, 2016

rayleigh commented Oct 11, 2016 • edited Loading

bob-carpenter left a comment

Choose a reason for hiding this comment

rayleigh commented Sep 28, 2016 •

edited

Loading

rayleigh commented Sep 28, 2016 •

edited

Loading

rayleigh commented Oct 11, 2016 •

edited

Loading

rayleigh commented Oct 11, 2016 •

edited

Loading