-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Generalised unary vector function framework #1558
Generalised unary vector function framework #1558
Conversation
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
I've removed the This was me misunderstanding the implementation of |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99) |
*/ | ||
template <typename T, require_t<is_fvar<scalar_type_t<T>>>...> | ||
inline auto log_softmax(T&& x) { | ||
return apply_vector_unary<T>::apply(std::forward<T>(x), [&](auto& alpha) { |
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.
Perfect forwarding brings no benefits here. This would work as well with no additional copys and the code is a bit shorter:
inline auto log_softmax(const T& x) {
return apply_vector_unary<T>::apply(x, [&](auto& alpha) {
I think the same holds true for every other place in this PR that uses perfect forwarding.
Perfect forwarding is better than just const refs only if a variable can outlive the function in question. For example if the function can directley return the variable or construct some object that holds the variable.
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.
Perfect forwarding is better than just const refs only if a variable can outlive the function in question. For example if the function can directley return the variable or construct some object that holds the variable.
Can you link me to a doc that talks about that? My understanding of PF is that it tells the compiler "I'm okay with moving ownership of this memory and leaving the original object in an undefined state"
In particular, for the below think about a recursive function that can have a long ref stack that the compiler probably can't sort through. PF gives the compiler the ability to move ownership of that memory. So we don't have to worry about whether the compiler will have alias issues when deciding to inline recurses.
For example if the function can directly return the variable or construct some object that holds the variable.
The CPP guides have a lot on this with a nice little table in F.15 has some good heuristics
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing
In particular this seems to recommend Andrew keep it as a forwarding reference
It feels like the above func is just something we want to allow the compiler to move through if it's able
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.
Can you link me to a doc that talks about that?
Sorry, that is just my understanding and I a quick search does not find anything on when perfect forwarding is benefitial and when it is not. I can't think of any other example, but please correct me if I am wrong.
My understanding of PF is that it tells the compiler "I'm okay with moving ownership of this memory and leaving the original object in an undefined state"
I would say more accurate would be "I am CAPABLE of either moving a temporary object or using a reference to lvalue object." In this PR no function can actually move the parameter, so const refs are just as good.
PF gives the compiler the ability to move ownership of that memory. So we don't have to worry about whether the compiler will have alias issues when deciding to inline recurses.
I don't know anything about that. Can you link any doc or benchmark that supports this?
Thanks for the links to the guidelines. However I disagree with your conclusion that they suggest we should use perfect forwarding. The first link points to section that says simple means of parameter passing (which exclude perfect forwarding) should be used, unless benefits of something more advanced are demonstrated.
The second link suggests nothing on whether perfect forwarding should be used or not. It only explains what a function accepting a forwarding reference should do with it. It says that a function should be flagged (as violatin those guidelines) if it does anything with this argument except std::forward
-ing it exactley once.
I would add to this that std::forward
-ing an argument to function is only beneficial if that function either uses perfect forwarding or has two overloads accepting const lvalue ref and rvalue ref.
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 agree that we shouldn't clutter the code with perfect forwarding examples where they can't be used. This came up in another PR with autodiff types, which have no data content to move.
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.
@t4c1 at work but ill link some stuff tonight
Maybe we should write up an rfc to talk about what types and sizes should use certain argument types. I tend to be way to pf giddy
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.
This looks great! There are a few places it can be improved a bit and a question or two.
@andrjohns I will make a PR to your fork to fix these merge conflicts. This wont be completely trivial changes so I think a PR is needed. I missed that you are touching fwd files, else I would have hold off on the flattening. EDIT: It was actually more or less straightforward, so I just pushed the changes. Hopefully that is fine. |
…ter_flatten # Conflicts: # test/unit/math/prim/scal/fun/log_sum_exp_test.cpp
…ter_flatten # Conflicts: # stan/math/prim/mat.hpp
# Conflicts: # stan/math/fwd/arr.hpp # stan/math/fwd/mat/fun/log_sum_exp.hpp
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98) |
Thanks for sorting out those conflicts @rok-cesnovar! @t4c1 I'll update this PR (and the tests) once #1471 gets merged in. Thanks! |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
deriv += v[i].d_ * exp_vi; | ||
} | ||
return fvar<T>(log_sum_exp(vals), deriv / denominator); | ||
return log_sum_exp(x2, x1); |
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've removed a redundant definition here (and in the rev) header. Originally, there was a definition for log_sum_exp(const fvar<T>& x1, double x2)
and log_sum_exp(double x1, const fvar<T>& x2)
, but we can just have one definition and change the order of arguments as needed.
@t4c1 This is ready for another look-over |
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.
Fix the conflicts and a few details and this is good to go.
return max + std::log((x.array() - max).exp().sum()); | ||
template <typename T, require_t<std::is_arithmetic<scalar_type_t<T>>>...> | ||
inline auto log_sum_exp(const T& x) { | ||
return apply_vector_unary<T>::reduce(x, [&](auto& v) { |
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.
return apply_vector_unary<T>::reduce(x, [&](auto& v) { | |
return apply_vector_unary<T>::reduce(x, [&](const auto& v) { |
With no perfect forwarding this can be const. Same for all other lambdas you introduced.
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.
Looks good. I think we have feture freeze now, so merging needs to wait until release.
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@rok-cesnovar I'm guessing this means that the segfaults have been fixed? Is this safe to merge? |
I think either way we need to wait until next release since we are in code freeze atm |
Good catch, forgot about that part |
Yes, the segfault were resolved (see #1615), but we have to wait until Sunday/Monday to merge after the release. |
Summary
This pull introduces a framework for generalising unary vector functions (e.g.
log_sum_exp
orlog_softmax
) to work with Eigen column and row vectors,std::vectors
, and containers of these.The motivations behind the framework were discussed in #1425, and the design document for the framework is here. There has also been additional discussion over in the forums.
A new vectorisation framework,
apply_vector_unary
, is proposed. The framework has two functions to address the different types of vector functions:apply_vector_unary<T>::apply()
for:f(vector) -> vector
apply_vector_unary<T>::reduce()
for:f(vector) -> scalar
This pull also introduces an example use of each type of this vectorisation:
log_softmax
(apply()
)log_sum_exp
(reduce()
)For each of these functions, their
prim
,rev
, andfwd
definitions have been re-written in the new framework, and their associated tests expanded.Tests
Both prim and mix tests have been added to check that the same values and derivatives are returned across the new vector inputs.
Side Effects
I had to update the templating in one header from
Eigen::Matrix<T, R, C>
toEigen::MatrixBase<Derived>
so that they could work withEigen::Map
inputs:matrix_vari
Checklist
Math issue Use Eigen::Map to replace arr functions #1425
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
./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