-
Notifications
You must be signed in to change notification settings - Fork 2
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
Initial implementation of transition_hyperparameters #25
Conversation
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.
The math looks good! Left a couple minor questions.
return log_numer - log_denom; | ||
} | ||
double logp_score() const { | ||
double v1 = lbeta(s + alpha, N - s + beta); |
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.
Our current declaration / implementation of lbeta
in util_math.hh
and util_math.cc
declare the arguments as int
s. My compiler issued warnings that floating-point numbers were being coerced. I think now that we have hyperparameters that can be non-integer values, we'll want to change the argument types of lbeta
(I think the implementation should stay the same).
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.
Fixed.
for (double alphat : ALPHA_GRID) { | ||
alpha = alphat; | ||
double lp = logp_score(); | ||
if (!std::isnan(lp)) { |
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.
Do you have a sense of when nans are showing up?
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.
None in dirichlet_categorical, at least with the current unit test, but I did see them previously in BetaBernoulli's transition_hyperparamters.
Huh. When I rerun that now, I no longer see any Nan's. I wonder if the above lbeta change fixed them.
Only on the Distribution side (I.e., not hooked up to hirm.cc yet) and only for BetaBernoulli for now.