Reduce usage of sampler #1936

torfjelde · 2023-01-13T23:06:37Z

Long-term we want to move away from using DynamicPPL.Sampler to decide which variables are being sampled, etc.
This PR reduces the explicit accessing and some usage of DynamicPPL.Sampler as one step towards this.
In addition, due to how DynamicPPL.AbstractSampler is handled differently for SimpleVarInfo and VarInfo, the current design of LogDensityFunction (which requires a sampler) means that we cannot use SimpleVarinfo in this regard (SimpleVarInfo + AbstractSampler results in always sampling variables, hence making it useless for computing gradients, etc.).

src/Turing.jl

devmotion · 2023-01-13T23:32:20Z

src/Turing.jl

+# Convenient for end-user.
+function LogDensityfunction(
+    model::DynamicPPL.Model,
+    varinfo::DynamicPPL.AbstractVarInfo=DynamicPPL.VarInfo(model),
+    context::DynamicPPL.AbstractContext=DynamicPPL.DefaultContext(),
+)
+    return LogDensityFunction(model, varinfo, context)
+end


Should this be added at this point? IIRC it's not exported and not suggested that users use it (I think it's not even documented).

IMO yeah; in fact, I kind of want to export it.
Now that we're adopting the LogDensityProblems.jl interface more aggressively, I also find myself using it very often. I also very much like it because most users will find the "full" description necessary to run the model, i.e. model + varinfo + context too "scary". If we can say "if you want to play around with your model, you can convert it to a simple thing which is compatible with LogDensityProblems", I think that's quite nice.

I like it as well and I think we want to expose it somehow. My main questions are just whether the right time to do is during the refactoring and whether we want to expose it via a different API. For instance, we could also use something like

logjoint(model, ::Type{T}=VarInfo) where {T<:AbstractVarInfo} = LogDensityFunction(model, T(model), DefaultContext()) logprior(model, ::Type{T}=VarInfo) where {T<:AbstractVarInfo} = LogDensityFunction(model, T(model), PriorContext()) loglikelihood(model, ::Type{T}=VarInfo) where {T<:AbstractVarInfo} = LogDensityFunction(model, T(model), LikelihoodContext())

and keep LogDensityFunction an internal detail. Maybe that would be even easier for users.

Though I do like this interface, I feel like we still need to inform the end-user about it and might as well export it given that they'll be seeing it "a lot"?

I'm not sure if they have to know about LogDensityFunctions at all if logjoint etc would be the API. The only relevant point for thr uaers would be (and should be documented) that these functions return something that follows the LogDensityProblems interface. Fields, names etc. should not matter for users.

This constructor is actually quite a bit annoying as it doesn't let you re-use an existing AbstractVarInfo 😕

It was just the first thing I thought of, maybe there are better approaches. But maybe that's also an indication that a separate PR and discussion about the API could be uaeful.

Could always add make_logjoint, etc.?

I agree with a separate PR, but worried it's going to delay exposing the LogDensityProblems.jl-functionality to the users. But I'll make separate PR for now, and if it takes too long we can just export LogDensityFunction 🤷

Or something like logjointof etc., similar to (log)densityof.

On a second thought though, I think generally (and particularly when dealing with other model type) it would be nice if there would be some curried form of logjoint etc. that gives you a function that you can evaluate for different variables and that would be consistent with the exiating logjoint etc. And generally there one would only want to provide the variables when evaluating the curried form. Maybe one could add a keyword argument for the LogDensityFunction case (similar to e.g. how one can adjust behaviour of AD backends in LogDensityProblemsAD by providing an example of the values that log density function will be evaluated with). But to summarize, to me the desire to provide VarInfo(model) when constructing the LogDensityFunction seems very specific to this case (and maybe not something many users would want to do?) but not something you would want to do for every kind of models, and hence maybe it should not be first-class part of a generic API but a bit more specific to this case such as maybe a keyword argument.

src/Turing.jl

devmotion · 2023-01-13T23:40:16Z

src/essential/ad.jl

 getADbackend(spl::Sampler) = getADbackend(spl.alg)
 getADbackend(::SampleFromPrior) = ADBackend()()


Are these still needed? Or could we remove them and specialize getADbackend(::SamplingContext{<:Sampler}) and getADbackend(::SamplingContext{SampleFromPrior})? I think mentally it's easier if getADbackend only operates on contexts.

Yeah I can try doing that! I was worried this would break too many tests and discourage me from completing the PR, but now that the tests are passing I can see if it's worth it.

src/essential/ad.jl

src/Turing.jl

src/inference/mh.jl

src/modes/ModeEstimation.jl

coveralls · 2023-01-14T01:52:25Z

Pull Request Test Coverage Report for Build 3935194490

0 of 60 (0.0%) changed or added relevant lines in 6 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage remained the same at 0.0%

Changes Missing Coverage	Changed/Added Lines	%
src/inference/emcee.jl	1	0.0%
src/inference/ess.jl	1	0.0%
src/inference/mh.jl	4	0.0%
src/essential/ad.jl	6	0.0%
src/modes/OptimInterface.jl	13	0.0%
src/modes/ModeEstimation.jl	35	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/modes/OptimInterface.jl	1	0%
src/inference/mh.jl	2	0%
src/modes/ModeEstimation.jl	4	0%

Totals
Change from base Build 3919842744:	0.0%
Covered Lines:	0
Relevant Lines:	1422

💛 - Coveralls

codecov · 2023-01-14T01:54:57Z

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (b8542e2) compared to base (f61bb69).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1936   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1432    1422   -10     
======================================
+ Misses       1432    1422   -10

Impacted Files	Coverage Δ
src/Turing.jl	`0.00% <ø> (ø)`
src/essential/ad.jl	`0.00% <0.00%> (ø)`
src/inference/emcee.jl	`0.00% <0.00%> (ø)`
src/inference/ess.jl	`0.00% <0.00%> (ø)`
src/inference/mh.jl	`0.00% <0.00%> (ø)`
src/modes/ModeEstimation.jl	`0.00% <0.00%> (ø)`
src/modes/OptimInterface.jl	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: David Widmann <[email protected]>

yebai

Good idea to phase out filtering VarInfo variables by sampler, the new context mechanism is strictly better, I think.

yebai · 2023-01-14T18:29:16Z

src/essential/ad.jl

+getADbackend(::DynamicPPL.IsLeaf, ctx::DynamicPPL.AbstractContext) = ADBackend()()
+getADbackend(::DynamicPPL.IsParent, ctx::DynamicPPL.AbstractContext) = getADbackend(DynamicPPL.childcontext(ctx))

 function LogDensityProblemsAD.ADgradient(ℓ::Turing.LogDensityFunction)


Most codes in this ad file can be safely transferred to DynamicPPL too. One benefit is that users of DynamicPPL get the gradient feature of LogDensityFunction without loading the Turing package.

Yup, though I think I'll leave it here for the time being just because I want to keep the PRs simple.

…ngLang/Turing.jl into torfjelde/less-sampler-more-context

Project.toml

test/Project.toml

Co-authored-by: Hong Ge <[email protected]>

Related to TuringLang/Turing.jl#1936

devmotion · 2023-01-15T15:12:51Z

src/inference/mh.jl


    # Make a new transition.
-    densitymodel = AMH.DensityModel(Turing.LogDensityFunction(vi, model, spl, DynamicPPL.DefaultContext()))
+    densitymodel = AMH.DensityModel(


Isn't it easier to use a LogDensityModel? Then you don't need Fix1.

Wait, does that work though? AFAIK we don't support wrapping something that implements LogDensityModel in AMH.DensityModel?

We don't want to wrap it in a DensityModel, do we? step seem to support LogDensityModels: https://github.com/TuringLang/AdvancedMH.jl/blob/1638f068261f936141404e232dd7f099b2cdde95/src/mh-core.jl#L74-L113

This didn't work unfortunately. It seems we're hitting https://github.com/TuringLang/AdvancedMH.jl/blob/1638f068261f936141404e232dd7f099b2cdde95/src/AdvancedMH.jl#L69 instead of https://github.com/TuringLang/AdvancedMH.jl/blob/1638f068261f936141404e232dd7f099b2cdde95/src/AdvancedMH.jl#L72. Also do you know the reasoning behind defining one for both DensityOrLogDensityModel and LogDensityModel? Why isn't it DensityModel and LogDensityModel separately?

EDIT: Looking at git-blame, it seems like I tried to do a replace and was a bit too aggressive 😅

devmotion · 2023-01-15T17:38:25Z

src/Turing.jl

 import AdvancedVI
+using DynamicPPL: DynamicPPL, LogDensityFunction
 import DynamicPPL: getspace, NoDist, NamedDist
 import LogDensityProblems


Maybe this can be removed now?

Suggested change

import LogDensityProblems

Still need it for all the optimation related stuff.

This reverts commit 596b0b2.

torfjelde added 4 commits January 13, 2023 22:52

initial work on using less of sampler and more of context

affc71d

fixed optim and ad

e5ff14f

fixed ESS and MH

cf295f1

bump version

2024348

torfjelde requested a review from devmotion January 13, 2023 23:08

torfjelde commented Jan 13, 2023

View reviewed changes

src/Turing.jl Outdated Show resolved Hide resolved

fixed typo

4e38fd0

devmotion reviewed Jan 13, 2023

View reviewed changes

moved all LogDensityProblems related to DPPL

c986828

torfjelde mentioned this pull request Jan 14, 2023

[Merged by Bors] - Moved LogDensityFunction from Turing to DPPL TuringLang/DynamicPPL.jl#447

Closed

Apply suggestions from code review

67fda5e

Co-authored-by: David Widmann <[email protected]>

yebai approved these changes Jan 14, 2023

View reviewed changes

yebai reviewed Jan 14, 2023

View reviewed changes

torfjelde added 2 commits January 14, 2023 19:31

Merge branch 'master' into torfjelde/less-sampler-more-context

8833092

Merge branch 'torfjelde/less-sampler-more-context' of github.com:Turi…

d43d6fb

…ngLang/Turing.jl into torfjelde/less-sampler-more-context

yebai reviewed Jan 14, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

torfjelde commented Jan 14, 2023

View reviewed changes

test/Project.toml Outdated Show resolved Hide resolved

Apply suggestions from code review

093cd40

Co-authored-by: Hong Ge <[email protected]>

bors bot pushed a commit to TuringLang/DynamicPPL.jl that referenced this pull request Jan 14, 2023

Moved LogDensityFunction from Turing to DPPL (#447)

6d5dc40

Related to TuringLang/Turing.jl#1936

torfjelde added 3 commits January 15, 2023 10:45

make compat bounds correct

260b498

fixed bug in MH

3505c58

fixed MH again

21e922b

devmotion reviewed Jan 15, 2023

View reviewed changes

fixed Emcee

65bb1e2

devmotion reviewed Jan 15, 2023

View reviewed changes

torfjelde added 3 commits January 16, 2023 11:10

fixed emcee

edf172a

fixed broken tests and removed mentionings of sampler in optimization

6e5d304

fixed bug in optim

88d53d7

torfjelde added 7 commits January 16, 2023 15:31

added tests for demo models

b19f0aa

fixed missing support for certain models in optim

06875d5

fixed type

6cee95e

fix

5ec0a0d

use LogDensityModel instead of wrapping in Base.Fix

596b0b2

Revert "use LogDensityModel instead of wrapping in Base.Fix"

9ebdf77

This reverts commit 596b0b2.

disable failing unsupported models for reverse mode AD frameworks

b8542e2

yebai approved these changes Jan 17, 2023

View reviewed changes

torfjelde merged commit a7c78ba into master Jan 17, 2023

delete-merged-branch bot deleted the torfjelde/less-sampler-more-context branch January 17, 2023 15:18

		getADbackend(spl::Sampler) = getADbackend(spl.alg)
		getADbackend(::SampleFromPrior) = ADBackend()()

Reduce usage of sampler #1936

Reduce usage of sampler #1936

Uh oh!

Conversation

torfjelde commented Jan 13, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3935194490

💛 - Coveralls

Uh oh!

codecov bot commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 14, 2023 •

edited

Loading

codecov bot commented Jan 14, 2023 •

edited

Loading

torfjelde Jan 16, 2023 •

edited

Loading