[Merged by Bors] - Fix initialization of parameters for algorithms that use SampleFromUniform
#232
Conversation
| # TODO: fix properly by using sampler and evaluation contexts | ||
| # This is a quick fix for https://github.com/TuringLang/Turing.jl/issues/1588 | ||
| # and https://github.com/TuringLang/Turing.jl/issues/1563 | ||
| # to avoid that existing variables are resampled | ||
| if _spl isa SampleFromUniform | ||
| model(rng, vi, SampleFromPrior()) | ||
| else | ||
| model(rng, vi, _spl) | ||
| end |
There was a problem hiding this comment.
Would it be better to maybe remove the check if it's SampleFromUniform here:
DynamicPPL.jl/src/context_implementations.jl
Line 128 in d2678d5
And instead we just provide a "resample!" or something with similar behavior as set_and_resample!(vi, ())?
There was a problem hiding this comment.
Or will it be difficult to understand the downstream effects this might have?
There was a problem hiding this comment.
In that case we could even allow init_params to be a NamedTuple and use set_and_resample! instead of initialize_parameters!.
There was a problem hiding this comment.
I think probably it will break downstream code if the sampler logic is changed. That's one motivation for temporarily introducing this workaround. The second motivation is that actually on the contrary it has been discussed to resample always when using a sampler, i.e., instead of not sampling with SampleFromUniform one would rather sample always also with e.g. SampleFromPrior. IMO this seems reasonable, in particular when one uses separate sampling and evaluation contexts since then it would be much clearer when the variables are affected by rerunning the model (and one could still avoid resampling).
There was a problem hiding this comment.
Yeah I get that; I just realized that what I mentioned above would also allow the use of NamedTuple as init_params which might be way more convenient for end-users. But maybe that's best resolved in a different PR once we have introduced the contexts you mentioned 👍
|
bors try |
|
bors r+ |
…niform` (#232) This PR is a quick fix for TuringLang/Turing.jl#1563 and TuringLang/Turing.jl#1588. As explained in TuringLang/Turing.jl#1588 (comment), the problem is that currently `SampleFromUniform` always resamples variables in every run, and hence also initial parameters provided by users are resampled in https://github.com/TuringLang/DynamicPPL.jl/blob/9d4137eb33e83f34c484bf78f9a57f828b3c92a0/src/sampler.jl#L80. As mentioned in TuringLang/Turing.jl#1588 (comment), a better long term solution would be to fix this inconsistency and use dedicated evaluation and sampling contexts, as suggested in #80.
bors
bot
changed the title
SampleFromUniformSampleFromUniform
This PR is a quick fix for TuringLang/Turing.jl#1563 and TuringLang/Turing.jl#1588.
As explained in TuringLang/Turing.jl#1588 (comment), the problem is that currently
SampleFromUniformalways resamples variables in every run, and hence also initial parameters provided by users are resampled inDynamicPPL.jl/src/sampler.jl
Line 80 in 9d4137e
As mentioned in TuringLang/Turing.jl#1588 (comment), a better long term solution would be to fix this inconsistency and use dedicated evaluation and sampling contexts, as suggested in #80.