Add reverse #1650
Add reverse #1650
Conversation
| std::reverse_copy(x.begin(), x.end(), rev.begin()); | ||
| return rev; | ||
| } | ||
|
|
There was a problem hiding this comment.
You could replace the three definitions with a single apply_vector_unary here:
template <typename T>
inline auto reverse(const T& x) {
return apply_vector_unary<T>::apply(x, [&](const auto& v) {
return v.reverse();
});
}
There was a problem hiding this comment.
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 reversed
There was a problem hiding this comment.
Good to know! But for std::vector, will this make a copy?
There was a problem hiding this comment.
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 an std::vector and avoid the need for an intermediary copy
There was a problem hiding this comment.
I should note that this would also implicitly allow for reverse with nested arrays
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 the T 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.
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:
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;
}
template <typename T, require_eigen_vector_t<T>>
inline auto reverse(const T& x) {
return x.reverse();
}
There was a problem hiding this comment.
No worries, if anything I learned about apply_vector_unary! :)
There was a problem hiding this comment.
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:
template <typename T, require_eigen_vector_t<T>>
inline auto reverse(const T& x) {
return x.reverse().eval();
}
There was a problem hiding this comment.
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.
|
I'm overreviewing this one because I feel very strongly it would be wrong to have |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
This add the implementation of
reversefor vectors, row vectors and arrays. Fixes #344.Tests
Added some basic prim and mix tests.
Side Effects
None.
Checklist
Math issue add reverse() function for arrays and vectors #344
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
./runTests.py test/unit)make test-headers)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested