Add vectorised select(), any(), and all() functions by andrjohns · Pull Request #2853 · stan-dev/math

andrjohns · 2022-12-09T13:07:38Z

Summary

This PR extracts the vectorised select() function introduced in this PR (which in turn is adding the currently OpenCL-only function to the rest of the Math library), and also introduces the any() and all() helper functions.

The select() function is used for ternary operations that are intended to be agnostic between scalars and containers. If the function is used with a mixture of scalars and containers, then the scalar is promoted to the same size and type as the container.

Tests

Tests are added for all combinations of scalar/container inputs, as well as error checking for mismatched container sizes.

Side Effects

N/A

Release notes

Added select() function for vectorised ternary operations, as well as the any() and all() boolean reduction functions

Checklist

Math issue Add select() and any() helper functions #2852
Copyright holder: Andrew Johnson

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)
- dependencies checks pass, (make test-math-dependencies)
- 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

andrjohns · 2022-12-12T15:23:59Z

I've also added the any() and all() functions to this PR since they were fairly simple in comparison (happy to split to separate PR though!)

SteveBronder · 2023-05-10T18:02:11Z

Ping me when this is ready for review!

andrjohns · 2023-05-11T06:16:28Z

Ping me when this is ready for review!

Thanks @SteveBronder! This should be good to look at now

andrjohns · 2023-05-11T06:19:38Z

@SteveBronder one more thing I've been thinking about, do we want to start requiring/recommending tuple compatibility for utility functions like these? (probably only really relevant for any(), and all() here)

SteveBronder · 2023-05-11T20:59:22Z

I'm not sure if it should be a requirement, but it would def be nice!

andrjohns · 2023-05-14T11:17:49Z

@SteveBronder just addressed your comments. I've also changed the handling for tuples in all()/any() to use math::for_each() if you wouldn't mind double-checking that you're happy with that over the previous std::vector<bool> initialiser approach

SteveBronder · 2023-05-15T14:12:19Z

+template <typename T_true, typename T_false,
+          require_all_stan_scalar_t<T_true, T_false>* = nullptr>
+inline auto select(const bool c, const T_true y_true, const T_false y_false) {
+  return c ? y_true : y_false;


Should we cast y_true / y_false to the same type explicitly or is it okay if we assume they are implicitly promotable?

I think we want to do this implicit vs. explicit promotion everywhere

SteveBronder · 2023-05-15T14:13:18Z

+  if (c) {
+    return T_true_plain(y_true);
+  } else {
+    return T_true_plain(y_false);
+  }


Suggested change

if (c) {

return T_true_plain(y_true);

} else {

return T_true_plain(y_false);

}

return c ? T_true_plain(y_true) : T_true_plain(y_false);

SteveBronder · 2023-05-15T14:45:35Z

+template <typename T_bool, typename T_true, typename T_false,
+          require_eigen_array_t<T_bool>* = nullptr,
+          require_all_stan_scalar_t<T_true, T_false>* = nullptr>


Should we be more restrictive with T_bool here like require_eigen_array_vt<is_integral, T_bool> (bool is integrable here)

https://godbolt.org/z/shn6hb3ca

SteveBronder

sry clicked approved by accident. Few qs above!

SteveBronder · 2023-05-25T20:01:28Z

I'll take a look at this today

SteveBronder

Mostly doc fixes but once that's done then I think good to merge!

SteveBronder · 2023-07-20T20:03:59Z

+ * @param x boolean input
+ * @return The input unchanged
+ */
+inline bool all(bool x) { return x; }


(optional) constexpr?

Also should this be templated to accept any integral type? Not sure if we would get warnings about downcasting? This applies to all the bool as input functions

SteveBronder · 2023-07-20T20:12:42Z

+ *
+ * Overload for Eigen types
+ *
+ * @tparam Eigen type of the input


Suggested change

* @tparam Eigen type of the input

* @tparam ContainerT A type derived from `Eigen::EigenBase` that has an `integral` scalar type

Double check the docs for all these

SteveBronder · 2023-07-20T20:16:30Z

+ * Return the second argument if the first argument is true
+ * and otherwise return the third argument.
+ *
+ * <code>select(c, y1, y0) = c ? y1 : y0</code>.


(optional) I just prefer using ticks here ` doxygen is able to parse that like it's markdown

SteveBronder · 2023-07-20T20:20:54Z

+template <
+    typename T_true, typename T_false,
+    typename T_return = return_type_t<T_true, T_false>,
+    typename T_true_plain = promote_scalar_t<T_return, plain_type_t<T_true>>,
+    typename T_false_plain = promote_scalar_t<T_return, plain_type_t<T_false>>,
+    require_all_container_t<T_true, T_false>* = nullptr,
+    require_all_same_t<T_true_plain, T_false_plain>* = nullptr>
+inline T_true_plain select(const bool c, const T_true y_true,
+                           const T_false y_false) {
+  check_matching_dims("select", "y_true", y_true, "y_false", y_false);
+  return c ? T_true_plain(y_true) : T_true_plain(y_false);
+}


(optional) it may be nice to have a signature for when T_true_plain is not constructable from a T_false_plain that has a static assert in it with a nice message.

SteveBronder · 2023-07-20T20:22:04Z

+inline T_true_plain select(const bool c, const T_true y_true,
+                           const T_false y_false) {
+  check_matching_dims("select", "y_true", y_true, "y_false", y_false);
+  return c ? T_true_plain(y_true) : T_true_plain(y_false);
+}


If these are containers should they be passed as references like

Suggested change

inline T_true_plain select(const bool c, const T_true y_true,

const T_false y_false) {

check_matching_dims("select", "y_true", y_true, "y_false", y_false);

return c ? T_true_plain(y_true) : T_true_plain(y_false);

}

inline T_true_plain select(const bool c, T_true&& y_true,

T_false&& y_false) {

check_matching_dims("select", "y_true", y_true, "y_false", y_false);

return c ? T_true_plain(std::forward<T_true>(y_true)) : T_true_plain(std::forward<T_false>(y_false));

}

SteveBronder · 2023-07-20T20:30:50Z

+          require_eigen_array_vt<std::is_integral, T_bool>* = nullptr,
+          require_all_stan_scalar_t<T_true, T_false>* = nullptr>
+inline auto select(const T_bool c, const T_true y_true, const T_false y_false) {
+  return c.unaryExpr([&](bool cond) { return cond ? y_true : y_false; }).eval();


(optional) I personally prefer we just pull in the names of the things we want into a lambda instead of using &

Suggested change

return c.unaryExpr([&](bool cond) { return cond ? y_true : y_false; }).eval();

return c.unaryExpr([y_true, y_false](bool cond) { return cond ? y_true : y_false; }).eval();

SteveBronder · 2023-07-20T20:32:06Z

+  check_consistent_sizes("select", "boolean", c, "y_true", y_true, "y_false",
+                         y_false);


(optional) Should the names here be left hand side and right hand side? Like can this error be propagated up to users of Stan?

andrjohns · 2023-08-06T18:42:29Z

@SteveBronder this is ready for another look

SteveBronder

Mostly Qs on the signatures for containers and scalars as well as some docs.

SteveBronder · 2023-08-07T18:48:08Z

+template <typename T, require_integral_t<T>* = nullptr>
+constexpr inline T all(T x) {
+  return x;
+}


We still want a bool return type right?

Suggested change

template <typename T, require_integral_t<T>* = nullptr>

constexpr inline T all(T x) {

return x;

}

template <typename T, require_integral_t<T>* = nullptr>

constexpr inline bool all(T x) {

return x;

}

SteveBronder · 2023-08-07T18:54:36Z

+ *
+ * `select(c, y1, y0) = c ? y1 : y0`.
+ *
+ * @tparam T_true type of the true argument


Suggested change

* @tparam T_true type of the true argument

* @tparam T_true A stan `Scalar` type

Generally for overloads try to have the type docs reflect the specific types for this overload

Please fix this for all the docs for the types

SteveBronder · 2023-08-07T18:55:49Z

+ * Return the second argument if the first argument is true
+ * and otherwise return the third argument. Eigen expressions are


I'd rephrase these slightly so it goes from left to right in terms of arguments

Suggested change

* Return the second argument if the first argument is true

* and otherwise return the third argument. Eigen expressions are

* If first argument is true return the second argument, else return the third argument

SteveBronder · 2023-08-07T19:10:09Z

+  return c
+      .unaryExpr(
+          [y_true, y_false](bool cond) { return cond ? y_true : y_false; })
+      .eval();


How is the return type decided here? I think we should do like the other functions and explicitly cast to our intended return type

Suggested change

return c

.unaryExpr(

[y_true, y_false](bool cond) { return cond ? y_true : y_false; })

.eval();

using ret_t = return_type_t<T_true, T_false>;

return c

.unaryExpr(

[y_true, y_false](bool cond) { return cond ? ret_t(y_true) : ret_t(y_false); })

.eval();

SteveBronder · 2023-08-07T19:11:55Z

+inline auto select(const T_bool c, const T_true y_true, const T_false y_false) {
+  check_consistent_sizes("select", "boolean", c, "left hand side", y_true,
+                         "right hand side", y_false);
+  return c.select(y_true, y_false).eval();


Same as before, I think we should be casting y_true and y_false to the same type before doing the select. idk how Eigen handles that under the hood and I'd rather just be explicit about it

andrjohns · 2023-08-08T07:22:53Z

@SteveBronder ready for another look

SteveBronder

Some very minor comments on the docs then I think this is good to go!

SteveBronder · 2023-08-09T16:24:03Z

+ * @tparam Type of container
+ * @param x Nested container of boolean inputs


docs do not match signature

SteveBronder · 2023-08-09T16:24:29Z

+ * Overload for a single boolean input
+ *
+ * @tparam T The type of integral input.
+ * @param x boolean input


Suggested change

* @param x boolean input

* @param x integral input

SteveBronder · 2023-08-09T16:25:05Z

+ *
+ * Overload for a single boolean input
+ *
+ * @tparam T The type of integral input.


Suggested change

* @tparam T The type of integral input.

* @tparam T Any type convertible to `bool`

SteveBronder

lgtm!

andrjohns and others added 7 commits December 9, 2022 13:50

Start tests

f656c2a

Finish select tests

4bdddfd

cpplint

bebe3bc

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

d3a0546

Add prim def to opencl header

a3010f1

Add any,all and tests

997384a

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

7c8368e

andrjohns changed the title ~~Add a vectorised select function~~ Add vectorised select(), any(), and all() functions Dec 12, 2022

Merge branch 'stan-dev:develop' into vectorised-select

4deb4bb

Remove unnecessary overload, update doc

32d42fd

andrjohns and others added 3 commits May 13, 2023 16:12

Add tuple overloads for any and all

35d7129

Merge commit '4623b7522140457592eaf7b1fb7375b3ac318475' into HEAD

e6e564b

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

3d7ecfc

SteveBronder requested changes May 14, 2023

View reviewed changes

Comment thread stan/math/prim/fun/select.hpp Outdated

Comment thread stan/math/prim/fun/select.hpp Outdated

Comment thread stan/math/prim/fun/select.hpp

andrjohns and others added 2 commits May 14, 2023 14:08

Update doc, more efficient tuple any all

2d2ef5e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

49b1806

SteveBronder approved these changes May 15, 2023

View reviewed changes

SteveBronder requested changes May 15, 2023

View reviewed changes

andrjohns and others added 6 commits May 15, 2023 18:00

Updates

aa38925

Merge commit '3789de62bab3b77b1009709554b5ac97ac5a54a3' into HEAD

5049525

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

8d2ea05

Add nested MatriXd test

a3822ec

Merge commit 'f40a2d118dc52a22d1fcb64e939e3e1739d7b391' into HEAD

bf1e034

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

4eb51ef

Missing headers

cccc4b0

andrjohns mentioned this pull request Jul 19, 2023

Vectorize everything with broadcasting #202

Closed

SteveBronder requested changes Jul 20, 2023

View reviewed changes

andrjohns and others added 6 commits August 6, 2023 16:27

Review comments

111165f

cpplint

2e82959

Merge commit '34881d4b8b4d6bea963d2d69e2d12da8f723fb64' into HEAD

910995b

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

5738ece

Forwarding

3706a13

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

6bd8723

SteveBronder requested changes Aug 7, 2023

View reviewed changes

andrjohns and others added 2 commits August 7, 2023 23:17

Review comments

c82075e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

6ac1db4

SteveBronder requested changes Aug 9, 2023

View reviewed changes

SteveBronder mentioned this pull request Aug 9, 2023

Feature/issue 2682 add 7 parameter ddm pdf #2822

Merged

5 tasks done

Doc updates

64728a0

SteveBronder approved these changes Aug 9, 2023

View reviewed changes

andrjohns merged commit 38289cd into stan-dev:develop Aug 11, 2023

andrjohns mentioned this pull request Aug 11, 2023

Add select() and any() helper functions #2852

Closed

	* @tparam Eigen type of the input
	* @tparam ContainerT A type derived from `Eigen::EigenBase` that has an `integral` scalar type

	return c.unaryExpr([&](bool cond) { return cond ? y_true : y_false; }).eval();
	return c.unaryExpr([y_true, y_false](bool cond) { return cond ? y_true : y_false; }).eval();

		check_consistent_sizes("select", "boolean", c, "y_true", y_true, "y_false",
		y_false);

	* @tparam T_true type of the true argument
	* @tparam T_true A stan `Scalar` type

		* Return the second argument if the first argument is true
		* and otherwise return the third argument. Eigen expressions are

	* Return the second argument if the first argument is true
	* and otherwise return the third argument. Eigen expressions are
	* If first argument is true return the second argument, else return the third argument

		* @tparam Type of container
		* @param x Nested container of boolean inputs

	* @tparam T The type of integral input.
	* @tparam T Any type convertible to `bool`

Conversation

andrjohns commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Side Effects

Release notes

Checklist

Uh oh!

andrjohns commented Dec 12, 2022

Uh oh!

SteveBronder commented May 10, 2023

Uh oh!

andrjohns commented May 11, 2023

Uh oh!

andrjohns commented May 11, 2023

Uh oh!

SteveBronder commented May 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrjohns commented May 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveBronder left a comment

Choose a reason for hiding this comment

Uh oh!

SteveBronder commented May 25, 2023

Uh oh!

SteveBronder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrjohns commented Aug 6, 2023

Uh oh!

SteveBronder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrjohns commented Aug 8, 2023

andrjohns commented Dec 9, 2022 •

edited

Loading