Compile Time Theta Default - #3213
Conversation
Sorry, something went wrong.
WardBrian
left a comment
There was a problem hiding this comment.
My overall thoughts are pretty neutral. It's nice that this avoids a runtime conditional, but it's only one conditional at the start of the optimization. On the other hand, I find the laplace_options_base struct much harder to read now, as we really don't semantically mean for it to be a templated struct, we just want it to either have or not-have the last member.
Sorry, something went wrong.
| if constexpr (is_eigen_v<ThetaVec>) { | ||
| return options.theta_0; | ||
| } else { | ||
| return Eigen::VectorXd::Zero(theta_size); | ||
| } |
There was a problem hiding this comment.
Thoughts on making this a member function of the options struct? so this call would just be options.initial_theta(theta_size);, and the function body would basically be this code (plus maybe an assert that the size is really correct and/or moving the above checks into this function)
Sorry, something went wrong.
There was a problem hiding this comment.
If we used it more than once I would say yes, but it is only used here so I'd rather just have the code here
Sorry, something went wrong.
There was a problem hiding this comment.
IMO it's less about the fact that it's used once and more about the fact that the immediately-evaluated-lambda trick is messy. But, I could live with it here
Sorry, something went wrong.
| template <typename Theta, typename = void> | ||
| struct laplace_options; | ||
|
|
||
| /* Initial value for theta. Defaults to 0s of the correct size if nullopt */ | ||
| std::optional<Eigen::VectorXd> theta_0{std::nullopt}; | ||
| template <typename Theta> | ||
| struct laplace_options<Theta, require_eigen_t<Theta>> : public laplace_options_base { | ||
| /* Value for user supplied initial theta */ | ||
| Theta theta_0{0}; | ||
| }; | ||
|
|
||
| template <typename Theta> | ||
| struct laplace_options<Theta, require_not_eigen_t<Theta>> : public laplace_options_base {}; | ||
|
|
||
| using laplace_options_default = laplace_options<void>; |
There was a problem hiding this comment.
Would it be possible just to have this be template <bool HasInitTheta> and then the two specializations just being false and true?
Something like
template <bool HasInitTheta>
struct laplace_options;
template <>
struct laplace_options<false> : public laplace_options_base {};
template <>
struct laplace_options<true> : public laplace_options_base {
/* Value for user supplied initial theta */
Eigen::VectorXd theta_0{0};
};
using laplace_options_default = laplace_options<false>;
using laplace_options_theta0 = laplace_options<true>;
seems cleaner to me than require/require_nots and a void
Sorry, something went wrong.
There was a problem hiding this comment.
It looks like this does work, see https://godbolt.org/z/GM4nqM81a
Sorry, something went wrong.
There was a problem hiding this comment.
I like it! Just updated
Sorry, something went wrong.
Summary
@WardBrian how do you feel about this alternative to #3209 ? Instead of an std::optional for theta_0 I added a template to laplace_options for the type of theta_0. Since we know at compile time whether we are using the default theta_0 we can save having to make an optional here.
Tests
No new tests
Checklist
Copyright holder: Simons Foundation
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