Improvements to `externalsampler` #2204

torfjelde · 2024-04-20T14:06:27Z

Currently there's no way of choosing an ad backend for externalsampler, nor to specify whether it requires constrained or unconstrained parameters. This PR adds those options.

In addition, the PR adds a default_adtype method to be more consistent across the codebase.

and choice of adtype

src/Turing.jl

src/mcmc/Inference.jl

yebai · 2024-04-20T14:16:06Z

Nice improvements to externalsampler!

default in ADTypes.jl

…vements' into torfjelde/external-sampler-improvements

coveralls · 2024-04-20T16:02:24Z

Pull Request Test Coverage Report for Build 8808209405

Details

0 of 29 (0.0%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage remained the same at 0.0%

Changes Missing Coverage	Changed/Added Lines	%
src/deprecated.jl	2	0.0%
src/mcmc/hmc.jl	2	0.0%
src/mcmc/Inference.jl	9	0.0%
src/mcmc/abstractmcmc.jl	16	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/mcmc/abstractmcmc.jl	1	0.0%

Totals
Change from base Build 8803479078:	0.0%
Covered Lines:	0
Relevant Lines:	1529

💛 - Coveralls

devmotion · 2024-04-20T20:50:38Z

src/Turing.jl

@@ -18,6 +18,10 @@ import StatsBase
 import Printf
 import Random

+using ADTypes: ADTypes
+
+default_adtype() = ADTypes.AutoForwardDiff()


Wouldn't a constant be simpler?

Suggested change

default_adtype() = ADTypes.AutoForwardDiff()

const DEFAULT_ADTYPE = ADTypes.AutoForwardDiff()

Actually did that first 😅

Buuuut using a method does allow an advanced user to actuallly overload the default AD backend that is used 🤷

I'm not completely convinced that this should be supported since it has such widereaching consequences. Some limited flexibility could be allowed by making the default a Preferences-constant that could be modified by users.

Made it a const as desired:)

devmotion · 2024-04-20T20:53:57Z

src/mcmc/Inference.jl

+    function ExternalSampler(sampler::S, adtype::AD, unconstrained::Bool=true) where {S<:AbstractSampler,AD<:ADTypes.AbstractADType}
+        return new{S,AD,unconstrained}(sampler, adtype)
+    end


Generally, the return type can't be inferred here. Maybe make unconstrained the first type parameter and only support

ExternalSampler{unconstrained}(sampler, adtype) where unconstrained = ExternalSampler{unconstrained,typeof(sampler),typeof(adtype)}(sampler, adtype) ExternalSampler(sampler, adtype) = ExternalSampler{true}(sampler, adtype)

Am aware, and am happy to make this change 👍 Buuuut the user will be using externalsampler to construct it anyways, which will never be inferrable either 😕

Buuut I agree with you;; we should at least have the option of using a type-inferrable constructor so will make the change 👍

Maybe use Val(true)/Val(false) in externalsampler to make it fully inferrable?

Maybe use Val(true)/Val(false) in externalsampler to make it fully inferrable?

Don't like this as it requires users to be familiar with what Val is, which doesn't seem worth it given that externalsampler will never be used in "hot" code (it's really only going to be used before sample(...), which is not type-stable anyways) 😕

Made the changes you wanted to the constructor (though leeft externalsampler as is)

Making use of Val for constructor ExternalSampler, but leaving externalsampler as is I think

devmotion · 2024-04-20T20:54:50Z

test/mcmc/abstractmcmc.jl

    f = DynamicPPL.LogDensityFunction(model)
    d = LogDensityProblems.dimension(f)
    return AdvancedMH.RWMH(MvNormal(Zeros(d), 0.1 * I))
 end

+# TODO: Should this go somewhere else?


Probably DynamicPPL I'd say?

Was thinking the same. Only thing holding me back is that it's unclear to me if this is indeed how we want to implement something like this 😕 It was a quick--and-dirty way of doing it, hence why I figured I'd just leave it here for now.

…tion

torfjelde · 2024-04-23T09:10:49Z

You happy with the current version @devmotion ?

torfjelde · 2024-04-23T09:28:08Z

I'll add one final thing to this PR: fix issues with initial params when working with unconstrained models.

torfjelde · 2024-04-23T11:30:09Z

Added initial params handling + testing. Should pass as usual, at which point we can merge:)

torfjelde · 2024-04-23T13:01:15Z

I'll merge this once tests have passed unless anyone has any immediate concerns 👍 Should go in the same release as #2197 as this is also technically breaking

torfjelde · 2024-04-23T18:30:16Z

Actually, nvm, this is not breaking 🤦 Will check that tests run after merge with master, and then will merge (unless there are objections)

src/Turing.jl

src/mcmc/Inference.jl

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

torfjelde · 2024-04-23T22:59:57Z

Accepted your changes @devmotion ; thanks!

yebai · 2024-04-28T16:18:48Z

@torfjelde, the PR passes all CI tests but seems to have introduced several CI errors after merging into master.

devmotion · 2024-04-28T22:44:51Z

I assume the errors are unrelated to this PR and caused by the latest release of AliasTables: LilithHafner/AliasTables.jl#48

devmotion · 2024-04-29T08:46:09Z

Should be fixed by AliasTables 1.1.1.

torfjelde added 4 commits April 20, 2024 15:04

added option of specifying if an ExternalSampler requires linking

88a2c22

and choice of adtype

use default_adtype everywhere to avoid accidental incorrect choices

3a9c5eb

fixed incorrect argument order

db916cb

fixed incorrect reference

a842874

torfjelde requested review from sunxd3, devmotion and yebai April 20, 2024 14:07

torfjelde commented Apr 20, 2024

View reviewed changes

src/Turing.jl Outdated Show resolved Hide resolved

torfjelde commented Apr 20, 2024

View reviewed changes

src/mcmc/Inference.jl Outdated Show resolved Hide resolved

Update src/mcmc/Inference.jl

3d023d0

yebai approved these changes Apr 20, 2024

View reviewed changes

torfjelde added 4 commits April 20, 2024 15:33

remove usage of chunksize=0 in favour of chunksize=nothing, as is

8f05031

default in ADTypes.jl

fixed incorrect refrences in externalsampler stepp

a0857a5

Merge remote-tracking branch 'origin/torfjelde/external-sampler-impro…

a60cf97

…vements' into torfjelde/external-sampler-improvements

added tests for new kwargrs to externalsampler

2099d90

removed accidental import

ce8edb9

devmotion reviewed Apr 20, 2024

View reviewed changes

torfjelde added 5 commits April 20, 2024 23:34

made default_adtype() a const DEFAULT_ADTYPE instead

befabe4

chagned constructor for ExternalSampler to allow type-stable construc…

93bf8ef

…tion

fixed docstring

526c8d1

use Val type to make constructor of ExternalSampler type-stable

0a9c244

error if not val with true or false is provided

78a0883

torfjelde added 4 commits April 23, 2024 10:41

handle initial_params properly in step for externalsampler

07161c2

forgot to link initial params

9ca6506

test setting of initial params in external samplers

079f0bf

fixed NUTS externalsampler test

7133de0

test initial parameters for external samplers properly

3338458

torfjelde added 2 commits April 23, 2024 14:41

losen tests a bit

b3ee8b8

Merge branch 'master' into torfjelde/external-sampler-improvements

5fb0fe8

devmotion reviewed Apr 23, 2024

View reviewed changes

src/Turing.jl Outdated Show resolved Hide resolved

src/mcmc/Inference.jl Outdated Show resolved Hide resolved

torfjelde commented Apr 23, 2024

View reviewed changes

src/mcmc/Inference.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

2dc319f

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

yebai merged commit 17ab13d into master Apr 28, 2024
11 checks passed

yebai deleted the torfjelde/external-sampler-improvements branch April 28, 2024 10:36

yebai mentioned this pull request Apr 28, 2024

Bump AdvancedPS version to 0.6.0 #2210

Merged

penelopeysm mentioned this pull request Aug 14, 2024

Julia 1.7 tests are failing #2305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to `externalsampler` #2204

Improvements to `externalsampler` #2204

torfjelde commented Apr 20, 2024

yebai commented Apr 20, 2024

coveralls commented Apr 20, 2024 •

edited

Loading

devmotion Apr 20, 2024

torfjelde Apr 20, 2024

devmotion Apr 20, 2024

torfjelde Apr 23, 2024

devmotion Apr 20, 2024

torfjelde Apr 20, 2024

devmotion Apr 20, 2024

torfjelde Apr 20, 2024

torfjelde Apr 20, 2024

torfjelde Apr 21, 2024

devmotion Apr 20, 2024

torfjelde Apr 20, 2024

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024 •

edited

Loading

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

yebai commented Apr 28, 2024

devmotion commented Apr 28, 2024

devmotion commented Apr 29, 2024

	default_adtype() = ADTypes.AutoForwardDiff()
	const DEFAULT_ADTYPE = ADTypes.AutoForwardDiff()

Improvements to externalsampler #2204

Improvements to externalsampler #2204

Conversation

torfjelde commented Apr 20, 2024

yebai commented Apr 20, 2024

coveralls commented Apr 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8808209405

Details

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024 • edited Loading

torfjelde commented Apr 23, 2024

torfjelde commented Apr 23, 2024

yebai commented Apr 28, 2024

devmotion commented Apr 28, 2024

devmotion commented Apr 29, 2024

Improvements to `externalsampler` #2204

Improvements to `externalsampler` #2204

coveralls commented Apr 20, 2024 •

edited

Loading

torfjelde commented Apr 23, 2024 •

edited

Loading