Add map-like helpers - #3346
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
WardBrian
left a comment
WardBrian
left a comment
There was a problem hiding this comment.
Thanks @florence-bockting -- a few comments on items that stood out to me
|
It might also be useful if |
|
Yes I agree. I'm cool with that either happening in this PR or another PR |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
| namespace internal { | ||
|
|
||
| template <typename F, typename Tuple, typename Callback> | ||
| inline decltype(auto) with_tuple_prefix(F& f, Tuple& tup, Callback&& callback) { |
There was a problem hiding this comment.
@SteveBronder am I reading this correctly that the user will write a function like
vector mapper(real mu, real sigma, vector x1, vector x2) and call it like
col_mapN((mu,sigma), x1, x2)?
That feels sort of counter-intuitive compared to the other variadic functions. If we swapped the tuple and the arg pack, so that you had to pass the matrices together as a tuple, that would make slightly more sense to me
There was a problem hiding this comment.
idt there is any precedent on this in particular, but I like having the matrices first and then everything else after, then unpacking. So the signature would look like the following.
vector mapper(vector vec1, vector vec2, real mu, real sigma)
and the user call site would look like
col_mapN(mapper, (matrix1, matrix2), mu, sigma)
There was a problem hiding this comment.
I refactored the API now according to the idea:
vector mapper(vector vec1, vector vec2, real mu, real sigma);
col_mapN(mapper, (matrix1, matrix2), mu, sigma);
Same pattern for mapN / row_mapN.
Great. I added a TODO in the PR description pointing to a (to-be-created) follow-up PR that focuses on supporting various input types for |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
Overall good, there are a few things like to fix before submit, but once these are done then I think we are good to go
| static_assert(tuple_size_v<Tuple> >= 2, | ||
| "mapN requires a tuple of at least two std::vector inputs."); |
There was a problem hiding this comment.
I would remove this. It would be fine for a user to have a size 1 or size 0 tuples.
There was a problem hiding this comment.
A size zero tuple means there are no containers to map over, right? Seems good to prevent, but we can also do that in stanc3
| return apply( | ||
| [&](auto&&... xs) { | ||
| static_assert((is_std_vector<std::decay_t<decltype(xs)>>::value && ...), | ||
| "mapN tuple elements must be std::vector."); |
There was a problem hiding this comment.
I would also remove this. We would want to stop this at the signature level by looking at the tuple elements. We actually do not have that but it would look something like the following
namespace internal {
template <template <class...> class TypeCheck, typename Tuple>
struct all_tuple_elements : std::false_type {};
template <template <class...> class TypeCheck, typename... Types>
struct all_tuple_elements<TypeCheck, std::tuple<Types...>>
: math::conjunction<TypeCheck<std::decay_t<Types>>...> {};
} // namespace internal
/**
* Require all elements of a tuple satisfy a type trait
* @tparam TypeCheck a type trait that accepts any number of template parameters
* @tparam Tuple a tuple
*/
template <template <class...> class TypeCheck, typename Tuple>
using require_all_tuple_elements_t
= require_t<internal::all_tuple_elements<TypeCheck, std::decay_t<Tuple>>>;Then we would update this signature like
template <typename F, typename Tuple, typename... Args,
require_all_tuple_elements_t<is_std_vector, Tuple>* = nullptr>| static constexpr const char* function = "mapN"; | ||
|
|
||
| return apply( | ||
| [&](auto&&... xs) { |
There was a problem hiding this comment.
Two things here for style purposes
- Since we have
xson the outside signatureTuple&& xs, we should try to give the argument in the signature here a different name. I usually use eitherxs_orxs_inner. Or you can rename the outer argument toiteratorsor something like that - Generally we do not like to use
[&]and prefer explicit captures. For reference, the[ ]part of[&](auto&&... xs) {...}is a capture list which captures objects from the outer scope to be used in the functions inner scope.
[&]grabs a reference to all locally scoped values
[=]makes a hard copy of locally scoped values (we almost never want to use that), and
[ ]does not bring in any locally scoped variables.
We prefer to use explicit reference captures so like
return apply([&args...] () {
(std::cout << ... << args) << '\n';
}();
}Below is a godbolt that shows all of these capture semantics you can play around with
https://godbolt.org/z/6WzTh9h69
| template <typename F, typename T1, typename T2, typename... Ts, | ||
| require_all_std_vector_t<T1, T2, Ts...>* = nullptr> | ||
| inline auto mapN(F&& f, T1&& x1, T2&& x2, Ts&&... xs) { | ||
| return mapN(std::forward<F>(f), std::forward_as_tuple(x1, x2, xs...)); | ||
| } |
There was a problem hiding this comment.
@WardBrian do you think we should support this signature? I think this could get confusing for users. If we are doing the tuple elements are what we iterate over then I think we should always enforce that is the api
There was a problem hiding this comment.
I'm not even entirely sure what this signature is -- everything is a vector, but there needs to be at least two of them?
There was a problem hiding this comment.
I believe it allows a nontuple version with no extra args. Let's remove this signatures
| result_row_t first_row = f(m_ref.row(0), args...); | ||
| if (first_row.size() == 0) { | ||
| return matrix_t(0, 0); | ||
| } | ||
| matrix_t result(n_rows, first_row.cols()); | ||
| result.row(0) = first_row; | ||
| for (Eigen::Index i = 1; i < n_rows; ++i) { | ||
| result_row_t row = f(m_ref.row(i), args...); | ||
| check_size_match(function, "columns of result", result.cols(), | ||
| "columns of returned row", row.cols()); | ||
| result.row(i) = row; |
There was a problem hiding this comment.
Instead of making a new vector to assign to each time you can reuse the first one
| result_row_t first_row = f(m_ref.row(0), args...); | |
| if (first_row.size() == 0) { | |
| return matrix_t(0, 0); | |
| } | |
| matrix_t result(n_rows, first_row.cols()); | |
| result.row(0) = first_row; | |
| for (Eigen::Index i = 1; i < n_rows; ++i) { | |
| result_row_t row = f(m_ref.row(i), args...); | |
| check_size_match(function, "columns of result", result.cols(), | |
| "columns of returned row", row.cols()); | |
| result.row(i) = row; | |
| // `eval(...)` forces evaluation | |
| auto row_i = eval(f(m_ref.row(0), args...)); | |
| if (row_i.size() == 0) { | |
| return matrix_t(0, 0); | |
| } | |
| matrix_t result(n_rows, row_i.cols()); | |
| result.row(0) = row_i; | |
| for (Eigen::Index i = 1; i < n_rows; ++i) { | |
| row_i = f(m_ref.row(i), args...); | |
| check_size_match(function, "columns of result", result.cols(), | |
| "columns of returned row", row.cols()); | |
| result.row(i) = row_i; |
5 participants
Summary
Fixes #3339
This PR adds elementwise and row-/columnwise map helpers for
std::vectorand Eigen matrices:map,mapN,row_map,col_map,row_mapN, andcol_mapN.Implementation details:
map(f, x, args...): apply f to each element of a std::vector, with optional shared argsmapN(f, xs, args...):xsis a tuple of ≥2 equal-sized std::vectors and appliesf(get<0>(xs)[i], get<1>(xs)[i], ..., args...)mapN(f, x1, x2, xs...): convenience function; equivalent tomapN(f, forward_as_tuple(x1, x2, xs...))row_map/col_map: same pattern over Eigen matrix rows/columnsrow_mapN/col_mapN: same zip pattern over ≥2 equal-dimension Eigen matrices (tuple + trailing shared args, plus convenience function)Shared arguments are trailing so call sites look like
mapN(f, (x1, x2), mu, sigma)with functor signaturef(x1_i, x2_i, mu, sigma).Other details
fis never usefully appliedstan/math/prim/functor.hppTests
test/unit/math/prim/functor/map_test.cppSide Effects
I am not aware of any.
Release notes
map,mapN,row_map,col_map,row_mapN, andcol_mapNfunctors for elementwise and rowwise/columnwise mapping over std::vector and Eigen matrices.Checklist
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/)
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)mapshould support besides std::vectors also eigen [row]vectors and eigen matrices (see Add map-like helpers #3346 (comment))