-
Notifications
You must be signed in to change notification settings - Fork 5
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
James.yang/nuts revamp #13
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.
Looks great. Huge amount of work, very impressive. Add a few more end-to-end tests and I think it's good.
@@ -11,12 +12,20 @@ struct Constant : util::VarExpr<Constant<ValueType>> | |||
Constant(value_t c) | |||
: c_{c} | |||
{} | |||
value_t get_value(size_t) const { | |||
value_t get_value(size_t = 0) const { |
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.
Defaulting this scares me a little because it changes the API. Is there any way this could be a problem?
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.
We're ignoring this param anyway. This allows users to simply call get_value()
. I think enforcing developers to pass in a number to get_value
even when it's not going to get used is very confusing.
|
||
size_t max_depth = 10; | ||
size_t seed = 4821; | ||
nuts(model, warmup, n_samples, n_adapt, seed, |
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.
I think we need some more end to end tests like this. Take some of the more complicated regression tests and incorporate them here for NUTS.
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.
I know I was tired yesterday. Updated.
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.
👯
No description provided.