-
Notifications
You must be signed in to change notification settings - Fork 26
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
Allowing using NamedTuple
as initial_params
#632
Conversation
Looks reasonable, maybe we can just move Can I get your permission to just commit to this PR? @torfjelde |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Indeed:)
Yep, go for it! |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Pull Request Test Coverage Report for Build 10038614257Details
💛 - Coveralls |
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.
@torfjelde I changed several lines of your code, could you give a quick look? The rationales are in the code review comments.
vi::AbstractVarInfo, initial_params, spl::Sampler, model::Model | ||
function set_values!!( | ||
varinfo::AbstractVarInfo, | ||
initial_params::AbstractVector{<:Union{Real,Missing}}, |
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 changed the array element type from Real
to Union{Real, Missing}
because we allow initialization vector with missing
in it.
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.
Really? In what scenario would this be used?
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.
Line 135 in dfdc155
chain = sample(model, sampler, 1; initial_params=[missing, -1], progress=false) |
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.
Hmm, not sure how I feel about that. I guess it means to sample that parameter from the prior? But that's so simple to do by hand anyways these days.
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 suppose so, although it made sense to me because the initialization vector need to be the same dimension as the model, so it makes sense for someone to say "I don't care about these" by setting them to missing
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.
Sure, but you can also just do
rand(Vector, model)
and alter those values 😕
Basically, it's a question of whether we want to maintain this or just leave it up to the user when it is a simple one-liner.
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.
Got your argument, 👍
But in this case it's not really a one-liner, as user also need to set individual values.
For models with small dimensions, current syntax can still be useful. I am for keeping this option.
src/sampler.jl
Outdated
end | ||
|
||
# if initialize with scalar, convert to vector | ||
function set_values!!(varinfo::AbstractVarInfo, initial_params::Real, spl::AbstractSampler) |
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 reason of adding this method is that, before this PR, initial_params
can be a scalar.
Real
is not concrete, but probably okay given this likely is not on the critical path.
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.
Was not aware we allowed this either 🤷 Are we testing this?
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.
Yeah
Line 87 in dfdc155
chain = sample(model, sampler, 1; initial_params=0.2, progress=false) |
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.
Honestly, I say we drop this. Seems unnecessarily complicated when it's just a difference between 1
and [1]
.
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 see @torfjelde's point of simplicity of interface, but it is a breaking change, which makes me wonder if the small effort of keeping it supported is worth it. If we do change it, maybe bundle it with some other breaking changes?
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.
Though it's technically breaking, I highly doubt this piece of code exists anywhere else, i.e. it would be a matter of bumping compat bounds 🤔
function set_values!!( | ||
varinfo::AbstractVarInfo, initial_params::NamedTuple, spl::AbstractSampler | ||
) | ||
initial_params = NamedTuple(k => v for (k, v) in pairs(initial_params) if v !== missing) |
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.
Not sure this is good, but changed to support a more uniform interface: allowing NamedTuple
with missing
values.
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.
Same as in last comment: when are we initializing using missing
?
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.
see reply above
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.
Mostly looks great, I had a few questions I raised in local comments.
src/sampler.jl
Outdated
end | ||
|
||
# if initialize with scalar, convert to vector | ||
function set_values!!(varinfo::AbstractVarInfo, initial_params::Real, spl::AbstractSampler) |
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 see @torfjelde's point of simplicity of interface, but it is a breaking change, which makes me wonder if the small effort of keeping it supported is worth it. If we do change it, maybe bundle it with some other breaking changes?
end | ||
|
||
function set_values!!( | ||
varinfo::AbstractVarInfo, initial_params::NamedTuple, spl::AbstractSampler |
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.
Are we happy with spl
not doing anything in this function? I get that we need to leave it there to keep a uniform interface, but could this end up being called with varinfo
that has not been sampled with spl
, which would then produce unexpected results?
@@ -892,6 +892,12 @@ Base.keys(vi::TypedVarInfo{<:NamedTuple{()}}) = VarName[] | |||
return expr | |||
end | |||
|
|||
# FIXME(torfjelde): Don't use `_getvns`. |
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.
Is this still relevant? Don't know why _getvns
should be shunned.
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.
It's just unnecessary; Base.keys
should just be it + IIRC _getvns
is overly complicated.
initial_params
with NamedTuple
NamedTuple
as initial_params
LGTM:) Unfortunately I seems I can't approve because I'm the creator of the PR 🙃 |
wait till #626 to merge, then I'll fix the versioning and potential errors |
Do we need to make this breaking? |
Ref: TuringLang/Turing.jl#2286
Quick and dirty implementation, but does the trick.
I probably shouldn't spend much time on this (have other PRs that I need to complete), but figured I'd just add this here in case someone else wants to complete it (maybe @sunxd3 or @mhauru has time) :)
Basically, it's just missing: