Add `state_from_transition`, `parameters` and `setparameters!!` #86

torfjelde · 2021-10-21T20:57:01Z

See #85.

Currently, almost every package implementing a AbstractSampler comes with their custom state and transition types. Since there currently are no "interface" or "guidelines" about what the types of state and transition should implement, it means that interaction across different samplers, e.g. composing two samplers from different packages, is annoying.

This PR introduces some methods which aims to alleviate this issue. In particular it sampler-implementations to also implement

parameters(transition) for the transition-type which returns the parameters in the sample, where by "parameters" I mean the realizations of the random variables we're targeting present in the transition, and
setparameters!! for the state-type which updates the current state of the Markov chain, thus likely (this might depend on the sampler) conditioning the sampler/kernel on the provided parameters.

Why parameters for transition but setparameters!! for state? A transition should (AFAIK) always contain a realization of the random variables we're sampling while the state might not, e.g. MH with a MvNormal(zeros(n), I) as proposal distribution (not dependent on current position/state/parameters). Of course there might be more properties one would like to use when converting from PackageA.Transition to PackageB.State, but this seems like the lowest common denominator that we can come up with. Maaaybe we should have the same for logdensity, i.e. logdensity(transition) and setlogdensity!!(state), but this might not always be available nor correspond directly to the same thing for both samplers. Might be worth including though. They could also obviously be implemented for both if the implementer so chooses.

But for the cases where you want to make use of more than just the parameters (and potentially logdensity), one can overload the state_from_transition directly (which is what should be used by packages making these "meta-samplers", e.g. composition), e.g. AdvancedHMC might want to implement state_from_transition(::AdvancedHMC.HMCState, ::AdvancedHMC.Transition) to not only update the parameters in the state but the entire PhasePoint from AdvnacedHMC.Transition, etc.

Thoughts?

codecov · 2021-10-21T21:04:27Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4dbcb3f) 97.37% compared to head (d9f8585) 96.42%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   97.37%   96.42%   -0.95%     
==========================================
  Files           8        8              
  Lines         305      308       +3     
==========================================
  Hits          297      297              
- Misses          8       11       +3

Files	Coverage Δ
src/AbstractMCMC.jl	`57.14% <0.00%> (-42.86%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devmotion · 2021-10-22T14:16:51Z

parameters(transition) for the transition-type which returns the parameters in the sample, where by "parameters" I mean the realizations of the random variables we're targeting present in the transition,

Maybe call it samples instead?

devmotion

I added some comments.

We should also add tests and update the documentation.

devmotion · 2021-10-22T14:19:26Z

src/AbstractMCMC.jl

@@ -79,6 +79,36 @@ The `MCMCSerial` algorithm allows users to sample serially, with no thread or pr
 """
 struct MCMCSerial <: AbstractMCMCEnsemble end

+"""
+    state_from_transiton(state, transition_prev[, state_prev])


This one seems a bit confusing - so one already needs a state to return it? Based on the name I would have assumed a function signature state_from_transition(transition) -> state.

Well if you already have the state, wouldn't it just be an identity call? i.e.

state_from_transition(state, a, b) = state

You could just dispatch to a different state type. For example, state=nothing would try to reconstruct the state from the transitions. Maybe? Not sure. Perhaps I am just playing devils advocate here.

Hmm, maybe update_from_transition!!(state, transition_prev, state_prev) is more descriptive?

Yo, whadda yah guys think?

Oh, sorry, my bad. Yeah your proposed name change is clearer, let's do that.

Or maybe just update!!? Given that we might also want to pass in state_prev?

Yeah, since we don't want to export any of these functions anyway I'm fine with update!!. Is a bit nicer than update_from_transition!! IMO, and as you say we might not only update based on the transition.

Cool, I'll make that change then 👍

src/AbstractMCMC.jl

devmotion · 2021-10-22T14:27:30Z

Maaaybe we should have the same for logdensity, i.e. logdensity(transition) and setlogdensity!!(state)

As you mention, the main problem is that it is not clear which logdensity different transitions and states refer to, so one needs some more fine-grained system e.g. some additional trait to distinguish between e.g. logdensity of prior or joint. I guess it's best to leave this for a separate PR and/or issue.

cpfiffer

I'm happy with it after David's comments are addressed, but in general I think this is slick. Thanks for the PR!

cpfiffer · 2021-10-22T16:16:25Z

src/AbstractMCMC.jl

@@ -79,6 +79,36 @@ The `MCMCSerial` algorithm allows users to sample serially, with no thread or pr
 """
 struct MCMCSerial <: AbstractMCMCEnsemble end

+"""
+    state_from_transiton(state, transition_prev[, state_prev])


Well if you already have the state, wouldn't it just be an identity call? i.e.

state_from_transition(state, a, b) = state

You could just dispatch to a different state type. For example, state=nothing would try to reconstruct the state from the transitions. Maybe? Not sure. Perhaps I am just playing devils advocate here.

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

torfjelde · 2021-10-23T11:09:34Z

Maybe call it samples instead?

Though I agree with "parameters" not being a good choice, "samples" can be a bit confusing IMO given all the other sample-related functions we have. Would values make sense, given that in this context it's often used synonymously with realizations of random variables?

As you mention, the main problem is that it is not clear which logdensity different transitions and states refer to, so one needs some more fine-grained system e.g. some additional trait to distinguish between e.g. logdensity of prior or joint. I guess it's best to leave this for a separate PR and/or issue.

Happy with this 👍

torfjelde · 2021-11-04T16:15:06Z

Do you have ideas for alternatives to the use of parameters @devmotion and @cpfiffer ? As I said in my previous comment, I think "samples" is kind of confusing too. Really what we want is "set the realizations of the random variables" but I think "realizations" is too unfamiliar for people.

I think I'm leaning towards values and setvalues!!.

cpfiffer · 2021-11-04T17:55:48Z

I dunno I feel like I wouldn't mind using realize or realize!!. Short and sweet, and I don't think it's that out of the way. If that doesn't seem right, I don't mind using values (though I will note that it's a base function override, which I don't care about overmuch).

torfjelde · 2021-11-17T12:44:21Z

I've added some docs + motivating example: https://turinglang.github.io/AbstractMCMC.jl/previews/PR86/api/#Interacting-with-states-of-samplers

(Btw, I love the fact that I can preview docs in the PRs; I'm guessing this is your doing @devmotion ? ❤️ )

cpfiffer

Mhm yep I love it. Could you move us up a major version?

torfjelde · 2021-11-18T02:05:19Z

I think the only thing that's left is the discussion of parameters vs. realizations vs. values

cpfiffer · 2021-11-18T16:47:37Z

I prefer realizations but values is more Julian -- let's go with that.

torfjelde · 2021-11-19T02:03:36Z

On additional thing we might want to consider adding: logdensity and setlogdensity!!, or something along those lines. This is because:

Almost all samplers will have a cached value of the logdensity (up-to some constant) of their target in their state.
Some samplers will make use of said cached value, e.g. AdvancedMH uses this in its accept/reject computation.

Of course, this can be dealt with on a case-by-case basis by simply defining updatestate!! explicitly, but this will be unnecessary in most cases if we have something along the lines of logdensity and setlogdensity!!.

What do you say @devmotion @cpfiffer ? Add or not add, that is the question.

torfjelde · 2021-11-19T02:16:05Z

Also, @devmotion you mentioned tests. Do you have anything particular in mind?

I have a test for the MixtureSampler outlined in the docs using AdvancedMH.jl, but that seems a bit "too heavy". At the same time, doing something like:

struct TestState
    params
    logp
end

setvalues!!(s::TestState) = ...

etc. seems quite useless 🤷

We could potentially take the test I have for the MixtureSampler and add a super-simple implementation of RWMH to the test-suite of AbstractMCMC? Not sure I'm too big a fan of this either though, as it'll be more like an example use-case rather than testing the functionality provided by updatestate!! etc.

torfjelde · 2021-12-04T21:46:16Z

On additional thing we might want to consider adding: logdensity and setlogdensity!!, or something along those lines. This is because:

Almost all samplers will have a cached value of the logdensity (up-to some constant) of their target in their state.

Some samplers will make use of said cached value, e.g. AdvancedMH uses this in its accept/reject computation.

Of course, this can be dealt with on a case-by-case basis by simply defining updatestate!! explicitly, but this will be unnecessary in most cases if we have something along the lines of logdensity and setlogdensity!!.

What do you say @devmotion @cpfiffer ? Add or not add, that is the question.

Yo boys @devmotion @cpfiffer! You got any thoughts on the above? AFAIK this is the only thing that's holding this PR back.

devmotion · 2021-12-04T22:24:24Z

src/AbstractMCMC.jl

+@doc """
+    values(transition)
+
+Return values in `transition`.
+"""
+Base.values


Do we really want to use Base.values here? I think adding this documentation should be considered type piracy and would only be allowed if we define it only for our own types as arguments (which we can't do here).

Unrelated, why did you use @doc?

Do we really want to use Base.values here? I think adding this documentation should be considered type piracy and would only be allowed if we define it only for our own types as arguments (which we can't do here).

Hmm, I do agree with this. I'm coming around to values not being the best idea.

Unrelated, why did you use @doc?

IIRC you need @doc here to be able to add the docstring? I.e. if you remove the @doc it won't be added to the docstring of Base.values.

cpfiffer · 2021-12-05T22:06:37Z

Yo boys @devmotion @cpfiffer! You got any thoughts on the above? AFAIK this is the only thing that's holding this PR back.

Woops, sorry, missed this one. Full support from me on this one!

devmotion · 2021-12-05T22:54:21Z

Ah I also missed this question. I think it would probably be useful but I would leave it for a separate PR. There were quite many discussions about logdensity functions recently and there was a push for unifying them throughout the Julia ecosystem. DensityInterface.jl defines a common interface for log density functions and their evaluations now that is used by e.g. Distributions, AbstractPPL, and (AFAIK) in the future also MeasureTheory. Hence IMO we should be a bit careful and not introduce a different logdensity function here, now that we unified them, but also make sure that it corresponds to the DensityInterface assumptions (if we use its interface).

torfjelde · 2021-12-05T23:30:34Z

Yeah I was actually looking at the newly introduced logdensity stuff today, so I agree that maybe this should be done in a separate PR 👍

torfjelde · 2021-12-07T01:56:19Z

I renamed values and setvalues!! to realize and realize!! as mentioned earlier.

Even if we're not completely happy with these names, we can instead just change these at a later stage.

torfjelde · 2021-12-07T02:06:10Z

Ah I also missed this question. I think it would probably be useful but I would leave it for a separate PR. There were quite many discussions about logdensity functions recently and there was a push for unifying them throughout the Julia ecosystem. DensityInterface.jl defines a common interface for log density functions and their evaluations now that is used by e.g. Distributions, AbstractPPL, and (AFAIK) in the future also MeasureTheory. Hence IMO we should be a bit careful and not introduce a different logdensity function here, now that we unified them, but also make sure that it corresponds to the DensityInterface assumptions (if we use its interface).

After looking into DensityInterface.jl I don't think there's anything that should be relevant for AbstractMCMC's functionality for setting and extracting log-probabilities from states/transitions. For the AbstractModel, etc., then it's def relevant though!

So maybe we should just introduce logprob or logtargetprob or maybe even just logtarget, and its corresponding set..!!? I'm realizing more and more that it's necessary to be able to transfer the log target-prob to the next state if we want to do something interesting. In fact, I think update!! state should optionally be able to take the AbstractModel that we're sampling from too so that it can re-evaluate the current state if need be.

EDIT: I added model as an argument to updatestate!! according to the last paragraph above.

EDIT 2: Maybe something like

"""
    target_logdensity(model, transition)

Return the log-density of the target `model` represented at `transition`.

Note that this does not necessarily compute the log-density of `model` at
`transition`, but may simply return a cached result from `transition`.
"""
function target_logdensity(model::AbstractModel, transition)
    return DensityInterface.logdensityof(model, realize(transition))
end

"""
    set_target_logdensity!!(state, logp)

Update the log-density in `state` by setting it to `logp`.

If `state` is mutable, this function mutates `state` directly and returns `state`.
Otherwise a new and updated instance of `state` is returned.
"""
function set_target_logdensity!! end

github-actions · 2023-10-24T13:58:51Z

src/AbstractMCMC.jl

+"""
+function realize end
+
+


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

added state_from_transition, parameters and setparameters!!

e2bdfb7

torfjelde requested review from cpfiffer and devmotion October 22, 2021 09:13

devmotion reviewed Oct 22, 2021

View reviewed changes

cpfiffer reviewed Oct 22, 2021

View reviewed changes

Update src/AbstractMCMC.jl

7fa8de0

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

torfjelde added 7 commits November 17, 2021 11:18

renamed state_from_transition to updatestate!!

0a4fd17

adhere to julia convention

28bdf91

added docs

86a7826

fixed docs

e19cea7

fixed docs

d86499f

added example for why updatestate!! is useful

bce436d

improved MixtureState example

21f4d56

further improvements to docs

de0e5b2

cpfiffer approved these changes Nov 17, 2021

View reviewed changes

torfjelde added 4 commits November 19, 2021 01:12

renamed parameters and setparameters!! to values and setvalues!!

23b9119

fixed typo in docs

b9f476c

fixed documenting values

f7b6096

improved and fixed some bugs in docs

4ca57b0

fixed typo in docs

abebd59

torfjelde mentioned this pull request Dec 4, 2021

Refactoring Glue Code TuringLang/Turing.jl#1735

Closed

devmotion reviewed Dec 4, 2021

View reviewed changes

torfjelde mentioned this pull request Dec 6, 2021

Reopening of previous PR TuringLang/DynamicPPL.jl#309

Merged

2 tasks

renamed values and setvalues!! to realize and realize!!

d1d4642

added model to updatestate!!

c6c9554

JaimeRZP mentioned this pull request Sep 22, 2023

Bringback old getparams TuringLang/Turing.jl#2086

Closed

Merge branch 'master' into tor/state-transition-related

d9f8585

github-actions bot reviewed Oct 24, 2023

View reviewed changes

src/AbstractMCMC.jl

"""

function realize end

Copy link

Contributor

github-actions bot Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

torfjelde mentioned this pull request Dec 4, 2023

New Gibbs sampler using condition TuringLang/Turing.jl#2099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `state_from_transition`, `parameters` and `setparameters!!` #86

Add `state_from_transition`, `parameters` and `setparameters!!` #86

torfjelde commented Oct 21, 2021 •

edited

codecov bot commented Oct 21, 2021 •

edited

devmotion commented Oct 22, 2021

devmotion left a comment

devmotion Oct 22, 2021

cpfiffer Oct 22, 2021

torfjelde Oct 23, 2021

torfjelde Nov 3, 2021

cpfiffer Nov 3, 2021

torfjelde Nov 4, 2021

devmotion Nov 4, 2021

torfjelde Nov 4, 2021

devmotion commented Oct 22, 2021

cpfiffer left a comment

cpfiffer Oct 22, 2021

torfjelde commented Oct 23, 2021

torfjelde commented Nov 4, 2021

cpfiffer commented Nov 4, 2021

torfjelde commented Nov 17, 2021

cpfiffer left a comment

torfjelde commented Nov 18, 2021

cpfiffer commented Nov 18, 2021

torfjelde commented Nov 19, 2021 •

edited

torfjelde commented Nov 19, 2021

torfjelde commented Dec 4, 2021

devmotion Dec 4, 2021

torfjelde Dec 5, 2021

cpfiffer commented Dec 5, 2021

devmotion commented Dec 5, 2021

torfjelde commented Dec 5, 2021

torfjelde commented Dec 7, 2021

torfjelde commented Dec 7, 2021 •

edited

github-actions bot Oct 24, 2023

Add state_from_transition, parameters and setparameters!! #86

Are you sure you want to change the base?

Add state_from_transition, parameters and setparameters!! #86

Conversation

torfjelde commented Oct 21, 2021 • edited

codecov bot commented Oct 21, 2021 • edited

Codecov Report

devmotion commented Oct 22, 2021

devmotion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devmotion commented Oct 22, 2021

cpfiffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torfjelde commented Oct 23, 2021

torfjelde commented Nov 4, 2021

cpfiffer commented Nov 4, 2021

torfjelde commented Nov 17, 2021

cpfiffer left a comment

Choose a reason for hiding this comment

torfjelde commented Nov 18, 2021

cpfiffer commented Nov 18, 2021

torfjelde commented Nov 19, 2021 • edited

torfjelde commented Nov 19, 2021

torfjelde commented Dec 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpfiffer commented Dec 5, 2021

devmotion commented Dec 5, 2021

torfjelde commented Dec 5, 2021

torfjelde commented Dec 7, 2021

torfjelde commented Dec 7, 2021 • edited

github-actions bot Oct 24, 2023

Choose a reason for hiding this comment

Add `state_from_transition`, `parameters` and `setparameters!!` #86

Add `state_from_transition`, `parameters` and `setparameters!!` #86

torfjelde commented Oct 21, 2021 •

edited

codecov bot commented Oct 21, 2021 •

edited

torfjelde commented Nov 19, 2021 •

edited

torfjelde commented Dec 7, 2021 •

edited