Add row vector, array and int_array construction utilities

[mcol]

Summary

This adds the code to implement the following function signatures:

• row_vector one_hot_row_vector(int K, int k)
• row_vector ones_row_vector(int K)
• row_vector spaced_row_vector(int K, real low, real high)
• row_vector zeros_row_vector(int K)
• real[] one_hot_array(int K, int k)
• real[] ones_array(int K)
• real[] spaced_array(int K, real low, real high)
• real[] zeros_array(int K)
• int[] one_hot_int_array(int K, int k)
• int[] ones_int_array(int K)
• int[] zeros_int_array(int K)

Fixes #692.

Tests

Added unit tests for each function.

Side Effects

None.

Checklist

• Math issue Container construction utilities #692

• Copyright holder: Marco Colombo

  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

[bob-carpenter]

Before reviewing, I have some comments on scope.

All of the constant_XXX functions are duplicates of existing rep_XXX functions. For instance, constant_int_array and const(_real)_array are currently overloaded as rep_array(double, int) and rep_array(int, int). const_vector and const_row_vector are rep_vector and rep_row_vector.

The naming rep_ is similar to that used in R.

The matrix versions are missing from this list. We currently have rep_matrix and 2D and 3D arrays as overloads of rep_array. There are also a bunch of other fixed matrix and vector constructs that'd be useful like unit matrix and uniform-simplex (or symmetric simplex), though they don't need to all be added as part of this PR.

set_XXX functions should not have set_ as a prefix as nothing is getting set.

constant_XXX functions should not have constant_ as a prefix because the result isn't constant in any sense. I mean constant_XXX(...) is not an lvalue, but as soon as you assign it, which you have to do to use it, it's not constant any more. This is why Eigen just uses Zero and One. Note that they do not use Zeros and Ones, but I'm OK with its usage in the functions here, though I'm also OK with one_array and zero_array.

[mcol]

I don't have any particular attachment to these functions or names, so I'm happy to remove those that duplicate existing functionality. I followed the indications in #692, and when reviewing #1620 none of this came up. So whatever you settle on here will potentially affect the other functions (I'm totally fine with it).

[bob-carpenter]

Thanks. And sorry about not catching this the first time. I've been doing far too many reviews these days. I have to slow down and be more careful. Yes, I think we should remove the constant_ functions because they're duplicates and they're not constant. I also think set_ should be removed from the names. Given that none of this has gone into the Stan language yet, I don't think it should be a problem.

…

On Jan 21, 2020, at 1:11 PM, Marco Colombo ***@***.***> wrote: I don't have any particular attachment to these functions or names, so I'm happy to remove those that duplicate existing functionality. I followed the indications in #692, and when reviewing #1620 none of this came up. So whatever you settle on here will potentially affect the other functions (I'm totally fine with it). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[mcol]

No problem at all, it's better to get these right while they are not exposed yet.

[bob-carpenter]

Thanks. And sorry again for not being sharper on the first review!

…

On Jan 21, 2020, at 2:02 PM, Marco Colombo ***@***.***> wrote: No problem at all, it's better to get these right while they are not exposed yet. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[bob-carpenter]

There are some necessary changes and some optional ones. I laid out a few test patterns that should be applied throughout (specifically to include the size 0 tests in the general loop).

It'd be fantastic if you could write an expect-std-vector-eq function like the matrix ones. We really need that badly either in this PR or in another one.

[bob-carpenter]

[optional]
Probably not worth refactoring, but a single implementation would work here. I realize there'd need to be two fronting functions to work easily with the Stan language, but the core implementation is shared. They'd probably need an enable-if to ensure the type is arithmetic.

That's true of most of these functions.

[mcol]

I'll pass on this for now.

[bob-carpenter]

This can just be return {K, 1}; You can always return curly braces around constructor args for the return type.

And same for next definition.

[mcol]

This doesn't work:

./stan/math/prim/fun/ones_array.hpp:19:11: error: non-constant-expression cannot be
      narrowed from type 'int' to 'double' in initializer list [-Wc++11-narrowing]
  return {K, 1};
          ^
./stan/math/prim/fun/ones_array.hpp:19:11: note: insert an explicit cast to silence
      this issue
  return {K, 1};
          ^
          static_cast<double>( )

If I use the static cast around K, it builds a vector of two elements.

[bob-carpenter]

Thanks for trying. I've never seen that error and it's suprrising in that I thought you could always replace return-type constructor calls with braces. It clearly requires a stricter match.

[bob-carpenter]

It does look like declaring const int K might also work, but I'd rather not sprinkle consts around.

[bob-carpenter]

Should use braces constructor return.

[mcol]

Same as in the ones case.

[mcol]

I've addressed your request, apart from those that I couldn't get to compile. To be honest those don't seem a big deal, as my code was not much longer anyway. The suggestion to merge the array/int array implementations is good, but there are a number of moving pieces that I'm not familiar with just yet, so I may revisit it in the future.