-
-
Notifications
You must be signed in to change notification settings - Fork 195
Add reverse #1650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Add reverse #1650
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f88631d
Add reverse
mcol 46c2558
Rename test
mcol b538c96
Add tests for row vector and array
mcol 432a722
Use apply_vector_unary to unify implementations
mcol 4df4edf
Revert to not using apply_vector_unary to disallow elementwise reverse
mcol 32dd89c
Add tests for nested containers
mcol a503e07
Add missing header
mcol 5d67cb9
Add eval() to avoid returning an Eigen expression
mcol File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
#ifndef STAN_MATH_PRIM_FUN_REVERSE_HPP | ||
#define STAN_MATH_PRIM_FUN_REVERSE_HPP | ||
|
||
#include <stan/math/prim/meta.hpp> | ||
#include <stan/math/prim/fun/Eigen.hpp> | ||
#include <algorithm> | ||
#include <vector> | ||
|
||
namespace stan { | ||
namespace math { | ||
|
||
/** | ||
* Return a copy of the specified array in reversed order. | ||
* | ||
* @tparam T type of elements in the array | ||
* @param x array to reverse | ||
* @return Array in reversed order. | ||
*/ | ||
template <typename T> | ||
inline std::vector<T> reverse(const std::vector<T>& x) { | ||
std::vector<T> rev(x.size()); | ||
std::reverse_copy(x.begin(), x.end(), rev.begin()); | ||
return rev; | ||
} | ||
|
||
/** | ||
* Return a copy of the specified vector or row vector | ||
* in reversed order. | ||
* | ||
* @tparam T type of container (vector or row vector) | ||
* @param x vector or row vector to reverse | ||
* @return Vector or row vector in reversed order. | ||
*/ | ||
template <typename T, typename = require_eigen_vector_t<T>> | ||
inline auto reverse(const T& x) { | ||
return x.reverse().eval(); | ||
} | ||
|
||
} // namespace math | ||
} // namespace stan | ||
#endif |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
#include <test/unit/math/test_ad.hpp> | ||
#include <vector> | ||
|
||
TEST(MathMixMatFun, reverse_vector) { | ||
auto f = [](const auto& x) { return stan::math::reverse(x); }; | ||
|
||
// 0 x 0 | ||
Eigen::VectorXd x0(0); | ||
mcol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
stan::test::expect_ad(f, x0); | ||
|
||
// 1 x 1 | ||
Eigen::VectorXd x1(1); | ||
x1 << 1; | ||
stan::test::expect_ad(f, x1); | ||
|
||
// 4 x 4 | ||
Eigen::VectorXd x(4); | ||
x << 1, 3, 4, -5; | ||
stan::test::expect_ad(f, x); | ||
} | ||
|
||
TEST(MathMixMatFun, reverse_row_vector) { | ||
auto f = [](const auto& x) { return stan::math::reverse(x); }; | ||
|
||
// 0 x 0 | ||
Eigen::RowVectorXd x0(0); | ||
stan::test::expect_ad(f, x0); | ||
|
||
// 1 x 1 | ||
Eigen::RowVectorXd x1(1); | ||
x1 << 1; | ||
stan::test::expect_ad(f, x1); | ||
|
||
// 4 x 4 | ||
Eigen::RowVectorXd x(4); | ||
x << 1, 3, 4, -5; | ||
stan::test::expect_ad(f, x); | ||
} | ||
|
||
TEST(MathMixMatFun, reverse_array) { | ||
auto f = [](const auto& x) { return stan::math::reverse(x); }; | ||
|
||
// 0 x 0 | ||
std::vector<double> x0(0); | ||
stan::test::expect_ad(f, x0); | ||
|
||
// 1 x 1 | ||
std::vector<double> x1{1}; | ||
stan::test::expect_ad(f, x1); | ||
|
||
// 4 x 4 | ||
std::vector<double> x{1, 3, 4, -5}; | ||
stan::test::expect_ad(f, x); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
#include <stan/math/prim.hpp> | ||
#include <test/unit/math/prim/fun/expect_matrix_eq.hpp> | ||
#include <gtest/gtest.h> | ||
#include <vector> | ||
|
||
TEST(MathFunctions, reverse_array) { | ||
using stan::math::reverse; | ||
|
||
std::vector<double> x0; | ||
EXPECT_EQ(0, reverse(x0).size()); | ||
|
||
std::vector<double> x1(5); | ||
EXPECT_FLOAT_EQ(x1[0], reverse(x1)[0]); | ||
|
||
std::vector<double> x{7, -1.2, 4, -3}; | ||
std::vector<double> rev{-3, 4, -1.2, 7}; | ||
std::vector<double> y = stan::math::reverse(x); | ||
for (int i = 0; i < x.size(); i++) { | ||
EXPECT_FLOAT_EQ(y[i], rev[i]); | ||
} | ||
|
||
std::vector<std::vector<double>> cont_x{x0, x1, x}; | ||
std::vector<std::vector<double>> cont_rev{x, x1, x0}; | ||
std::vector<std::vector<double>> cont_y = stan::math::reverse(cont_x); | ||
for (int i = 0; i < cont_x.size(); i++) { | ||
EXPECT_EQ(cont_y[i].size(), cont_rev[i].size()); | ||
for (int j = 0; j < cont_y[i].size(); j++) { | ||
EXPECT_FLOAT_EQ(cont_y[i][j], cont_rev[i][j]); | ||
} | ||
} | ||
} | ||
|
||
TEST(MathFunctions, reverse_vector) { | ||
using stan::math::reverse; | ||
|
||
Eigen::VectorXd x0(0); | ||
EXPECT_EQ(0, reverse(x0).size()); | ||
|
||
Eigen::VectorXd x1 = Eigen::VectorXd::Constant(1, 5); | ||
expect_matrix_eq(x1, reverse(x1)); | ||
|
||
Eigen::VectorXd x(4); | ||
x << 7, -1.2, 4, -3; | ||
Eigen::VectorXd rev(4); | ||
rev << -3, 4, -1.2, 7; | ||
expect_matrix_eq(rev, reverse(x)); | ||
|
||
std::vector<Eigen::VectorXd> cont_x{x0, x1, x}; | ||
std::vector<Eigen::VectorXd> cont_rev{x, x1, x0}; | ||
std::vector<Eigen::VectorXd> cont_y = stan::math::reverse(cont_x); | ||
for (int i = 0; i < cont_x.size(); i++) { | ||
expect_matrix_eq(cont_y[i], cont_rev[i]); | ||
} | ||
} | ||
|
||
TEST(MathFunctions, reverse_row_vector) { | ||
using stan::math::reverse; | ||
|
||
Eigen::RowVectorXd x0(0); | ||
EXPECT_EQ(0, reverse(x0).size()); | ||
|
||
Eigen::RowVectorXd x1 = Eigen::RowVectorXd::Constant(1, 5); | ||
expect_matrix_eq(x1, reverse(x1)); | ||
|
||
Eigen::RowVectorXd x(4); | ||
x << 7, -1.2, 4, -3; | ||
Eigen::RowVectorXd rev(4); | ||
rev << -3, 4, -1.2, 7; | ||
expect_matrix_eq(rev, reverse(x)); | ||
|
||
std::vector<Eigen::RowVectorXd> cont_x{x0, x1, x}; | ||
std::vector<Eigen::RowVectorXd> cont_rev{x, x1, x0}; | ||
std::vector<Eigen::RowVectorXd> cont_y = stan::math::reverse(cont_x); | ||
for (int i = 0; i < cont_x.size(); i++) { | ||
expect_matrix_eq(cont_y[i], cont_rev[i]); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace the three definitions with a single
apply_vector_unary
here:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should note that this would also implicitly allow for
reverse
with nested arrays (e.g.reverse(std::vector<VectorXd>)
) which returns an array with each vector reversedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know! But for
std::vector
, will this make a copy?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has the same cost as the current implementations, where the result is evaluated into a vector which is then returned.
Eigen::Map
is used to evaluate directly into anstd::vector
and avoid the need for an intermediary copyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be super confusing. I think there should be a reverse function that works the same for any type
T[]
, namely by reversing the elements. If theT
is a container type, I don't think it should switch to working elementwise.If there really needs to be a form of
reverse
that reverses containers within a list, I think it should get a different name.I feel very strongly that this pattern of behavior should hold everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, @mcol can you revert this change (sorry for the detour!). You could instead collapse the two Eigen definitions, so that you just have one definition for Eigen vectors (row or col) and one for std::vectors:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, if anything I learned about
apply_vector_unary
! :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be a pain, but that eigen definition should have had an
.eval()
at the end, otherwise it could cause the same issues as #1644:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! After posting, I wanted to make sure I wasn't around the bend on this one, so I took an informal poll of the non-computer scientists in my office (I won't name names) and they expected a top-level reverse only, not an elementwise one.