Call Eigen logistic function for inv_logit instead of custom implementation - #3155
Call Eigen logistic function for inv_logit instead of custom implementation#3155jachymb wants to merge 0 commit into
Conversation
Sorry, something went wrong.
|
Hi @jachymb — I believe this TODO was suggesting we use the .logistic method on matrices in a specific overload for them, not for the implementation for double. Right now, the prim version of this function just has two overloads, one for doubles and one for all containers that uses apply_scalar (which is, essentially, a for loop) To complete the TODO, there would be 3 overloads: double prim matrices (using .logistic) std::vectors (using apply_scalar like it currently would) The hardest part of doing this is generally writing the template bounds so that the correct version is always selected. If you’re running into trouble with that, we can try to help! |
Sorry, something went wrong.
|
Thanks for the commentary! How about something like this replacing the current vectorized version then? template <typename T, require_eigen_t<T>* = nullptr>
inline auto inv_logit(const T& x) {
return x.array().logistic();
}
template <typename T, require_std_vector_t<T>* = nullptr, require_not_eigen_t<T>* = nullptr>
inline auto inv_logit(const T& x) {
return apply_scalar_unary<inv_logit_fun, T>::apply(x);
}it seems to work when I add tests for std::vector<double> and stan::math::matrix_d does that look reasonable? |
Sorry, something went wrong.
|
That looks more-or-less correct. Because we want the autodiff overloads (which live in a different file) to also not intersect with these, I think you need to add a require_not_var_matrix_t to the eigen overload. I think you might also still need to keep the require_all_not_nonscalar_prim_or_rev_kernel_expression_t on one of those, but I'm also not super familiar with the openCL details, so it's probably best we ask @SteveBronder on that one |
Sorry, something went wrong.
| */ | ||
| template < | ||
| typename T, require_not_var_matrix_t<T>* = nullptr, | ||
| require_all_not_nonscalar_prim_or_rev_kernel_expression_t<T>* = nullptr> | ||
| template <typename T, require_eigen_t<T>* = nullptr> | ||
| inline auto inv_logit(const T& x) { | ||
| return apply_scalar_unary<inv_logit_fun, T>::apply(x); | ||
| return x.array().logistic(); | ||
| } |
There was a problem hiding this comment.
So right now this function works over all containers that are not var<Matrix> or var<MatrixCL> types. If we want a version for just doubles I would add three signatures. Two in this file and one in stan/math/rev/fun/inv_logit.hpp
// Version that works on Eigen types which have an arithmetic value type
template <typename T, require_eigen_vt<std::is_arithmetic, T> * = nullptr>
inline auto inv_logit(T&& x);
// Version that works on std::vector types and does apply scalar unary
template <typename T, require_std_vector_t<T> * = nullptr>
inline auto inv_logit(T&& x);Then in stan/math/rev/fun/inv_logit.hpp to add the signature
// Version that works with Eigen types with inner `var` value types
template <typename T, require_eigen_vt<is_var, T> * = nullptr>
inline auto inv_logit(T&& x);You can look at the other function in rev/fun/inv_logit.hpp to see how to write functions with our autodiff scheme. We also have a guide for adding new functions at the link below
https://mc-stan.org/math/md_doxygen_2contributor__help__pages_2getting__started.html
Sorry, something went wrong.
|
We already have several functions using Eigen's vectorised calls that could be a good guide on approaching this For example: |
Sorry, something went wrong.
|
Added the signatures suggested by SteveBronder and WardBrian, but to be honest, at this point I don't feel very confident I actually know what I'm doing :( I think I may need to backtrack a few steps to study the code & theroy a bit more. If this isn't it, feel free to close the PR or reuse any of my code in some other way. |
Sorry, something went wrong.
|
This code is 90% of the way there! I'll look at this on Monday |
Sorry, something went wrong.
| return exp_a / (1 + exp_a); | ||
| } | ||
| return inv(1 + exp(-a)); | ||
| return Eigen::internal::scalar_logistic_op<double>()(a); |
There was a problem hiding this comment.
Revert this and leave it as is
Sorry, something went wrong.
|
@jachymb I was trying to update your branch, but something odd happened and now github will not let me reopen your PR? I very much apologize, your PR was very close! I think the issue was that you commited to your forks develop branch. Normally its good practice to make a new branch for a feature with git checkout -b my-feature-name
|
Sorry, something went wrong.
My first time trying to contribute. I tried to resolve an old TODO - see #3154
Summary
Call Eigen logistic function for inv_logit instead of custom implementation.
I did not touch opencl/kernels/device_functions/inv_logit.hpp - not sure if it's necessary or how opencl works here.
Tests
Original tests pass. No new tests added.
Side Effects
I am not aware of any. There may perhaps be some numerical stability/precision differences, I didn't look into that.
Release notes
Call Eigen logistic function for inv_logit instead of custom implementation.
Checklist
Copyright holder: Me.
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
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested