Skip to content
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

Ambiguity between fwd and mix signatures for hessian() #3056

Open
andrjohns opened this issue Apr 28, 2024 · 2 comments
Open

Ambiguity between fwd and mix signatures for hessian() #3056

andrjohns opened this issue Apr 28, 2024 · 2 comments

Comments

@andrjohns
Copy link
Collaborator

Description

With the current signatures for hessian in fwd and mix, it is not possible to call the fwd implementation with double types.

mix/functor/hessian.hpp:

template <typename F>
void hessian(const F& f, const Eigen::Matrix<double, Eigen::Dynamic, 1>& x,
             double& fx, Eigen::Matrix<double, Eigen::Dynamic, 1>& grad,
             Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic>& H)

fwd/functor/hessian.hpp:

template <typename T, typename F>
void hessian(const F& f, const Eigen::Matrix<T, Eigen::Dynamic, 1>& x, T& fx,
             Eigen::Matrix<T, Eigen::Dynamic, 1>& grad,
             Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic>& H)

Because the scalar type is explicit in the mix signature and templated in the fwd signature, calls to hessian with double types will always resolve to the mix implementation. This makes testing/validation (or even just use as an alternative) a bit of a hurdle.

I think any kind of fix for this would imply a breaking change (changing function names/arguments), so could be bundled in the 5.0 release (@SteveBronder)?

Current Version:

v4.8.1

@SteveBronder
Copy link
Collaborator

I think this makes sense to fix. Which signatures do you want to add/change? We would put this in 5.0 breaking changes

@andrjohns
Copy link
Collaborator Author

I think we want to keep the mix implementation being called by default, since it's more efficient, but add a way to force the fwd impl to be called.

I can think of two ways:

  1. Rename the fwd version to hessian_fwd/hessian_fvar/etc
  2. Add a boolean template parameter, defaulting to true, such that hessian<false>(...) resolves to the fwd impl

Thoughts/preferences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants