add discrete wrapper #11
Conversation
include/autoppl/expr_builder.hpp
Outdated
| > > | ||
| inline constexpr auto discrete(ProbType&& p_expr) | ||
| { | ||
| using p_t = details::convert_to_param_t<ProbType>; |
There was a problem hiding this comment.
This converts value types, it doesn't convert containers.
There was a problem hiding this comment.
Yup do we need support for containers? I can write one if needed.
|
The lower-case |
include/autoppl/expr_builder.hpp
Outdated
| > > | ||
| inline constexpr auto discrete(ProbType&& p_expr) | ||
| { | ||
| using p_t = details::convert_to_param_t<ProbType>; |
There was a problem hiding this comment.
Yup do we need support for containers? I can write one if needed.
|
We pushed the current impl of discrete with the intention that we weren't really gonna use it with expressions, but just values. Expressions can have different types and using variadic templates with tuple is like the only way to do this. This gets difficult because you must know the index at compile-time. |
Signed-off-by: Jenny Chen <[email protected]>
| @@ -92,7 +92,7 @@ inline void mh_posterior__(ModelType& model, | |||
| std::discrete_distribution disc_sampler({alpha, 1-2*alpha, alpha}); | |||
| auto cand = disc_sampler(gen) - 1 + curr; // new candidate in curr + [-1, 0, 1] | |||
| // TODO: refactor common logic | |||
| if (dist.min() <= cand && cand <= dist.max()) { // if within dist bound | |||
| if (static_cast<int>(dist.min()) <= cand && cand <= static_cast<int>(dist.max())) { // if within dist bound | |||
There was a problem hiding this comment.
wait this is not the correct casting - this will cause round issues when dist is continuous distribution. What you could do is something like static_cast<value_t>(...) <= static_cast<value_t>(cand) and you can get value_t from util::dist_expr_traits<whatever the distribution type is>::value_t.
|
@all-contributors please add @jenchen1398 for code and design |
|
I've put up a pull request to add @jenchen1398! 🎉 |
No description provided.