Closed torfjelde closed 10 months ago
Attention: 2 lines
in your changes are missing coverage. Please review.
Comparison is base (
4dbcb3f
) 97.37% compared to head (3ed5314
) 96.87%. Report is 4 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Can you add tests of all new lines?
Will do (but tomorrow; am about to sleep)!
The names init_params and initial_state seem inconsistent, can we either use init or initial in both cases?
Agreed. Hmm, I guess I'll make it init_state
then? Personally prefer the initial_*
but given that one already exists, I guess it makes sense to add this (though this init_params
isn't even offiically supported, right?)
though this init_params isn't even offiically supported, right?
It's officially supported: It's clearly documented and used in downstream packages such as EllipticalSliceSampling, DynamicPPL, and Turing.
It's officially supported: It's clearly documented and used in downstream packages such as EllipticalSliceSampling, DynamicPPL, and Turing.
Yes, that I'm fully aware of! Just remembered seeing the following in the docs:
There is no "official" way for providing initial parameter values yet. However, multiple packages such as EllipticalSliceSampling.jl and AdvancedMH.jl support an init_params keyword argument for setting the initial values when sampling a single chain. To ensure that sampling multiple chains "just works" when sampling of a single chain is implemented, we decided to support init_params in the default implementations of the ensemble methods:
Which I took to mean "we haven't really made an explicit decision on how to support initial parameters, but because so many downstream packages use init_params
, we stay compatible with it".
But nonetheless, I'll change it to init_state
then :+1:
Generally, I think I'd prefer initial
but yeah, it involves more changes :shrug: I'm also not a big fan of the name params
, maybe initial_sample
would be better?
Since #120 requires a breaking release anyway, we could also include more breaking changes. Otherwise we could deprecate the keyword argument, maybe something like the following could work:
function f(...; init_params=nothing, initial_params=init_params)
if init_params !== nothing
if initial_params !== init_params
throw(ArgumentError("..."))
end
Base.deprecate("....", f)
end
...
end
@devmotion I just came across this again; given that we decided to scrap #120 in the end, what do you think of at least merging this? Being able to specify the initial state would be quite useful.
EDIT: Just remember you're on vacation! No need to rush this:)
I decided to just rip the band-aid off and go with renaming init_params
to initial_params
right away.
If we don't, we end up in a somewhat awkward scenario where we have to pass along both init_params
and initial_params
, which might also just break downstream step
.
Can we merge #126 first and then rebase this PR on it? Or maybe even directly rebase the PR on #126 and adjust the base branch of the PR?
Most certainly :+1:
Rebased and change base for PR.
I'll add some tests for the intial_state
stuff sometime today :+1:
Seems the PR breaks EllipticalSliceSampling? 🤔 Interesting, even though it won't cause any breakage in practice if we tag a breaking release.
Could it maybe be something related to init_params
-> initial_params
?
But yes, I'll bump the major version so won't break anything in practice:)
Yeah, I assume that's the reason: https://github.com/TuringLang/EllipticalSliceSampling.jl/blob/ca4babb2baba9008805bc8234a6fd182119e57dc/src/abstractmcmc.jl#L25 https://github.com/TuringLang/EllipticalSliceSampling.jl/blob/ca4babb2baba9008805bc8234a6fd182119e57dc/test/simple.jl#L40 I wonder though why other packages that currently use init_params
(AdvancedMH IIRC? Turing?) are not affected by this change. Maybe they are missing tests for init_params
/initial_params
?
Maybe they are missing tests for init_params/initial_params?
Very likely :confused:
This seems convenient to have, e.g. for resuming sampling, running special warm-up procedures.
EDIT: Note that this is now dependent on #126