Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Add TestUtils submodule #313

Closed
wants to merge 19 commits into from
Closed

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Aug 23, 2021

This PR adds a DynamicPPL.TestUtils submodule which is meant to include functionality to make it easy to test new samplers, new implementations of AbstractVarInfo, etc.

As of right now, this is mainly just a collection of models with equivalent marginal posteriors using the different features of DPPL, e.g. some are using .~, some are using @submodel, etc.

Eventually this should be expanded to be of more use, but more immediately this will be useful to test functionality in open PRs, e.g. #269, #309, #295, #292.

These models are also already used in Turing.jl's test-suite (https://github.com/TuringLang/Turing.jl/blob/9f52d75c25390b68115624b2e6cf464275a88137/test/test_utils/models.jl#L55-L56), so this PR would avoid the code-duplication + make it easier to keep things up-to-date.


# A collection of models for which the mean-of-means for the posterior should
# be same.
@model function demo1(x=10 * ones(2), ::Type{TV}=Vector{Float64}) where {TV}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use a bit more descriptive names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought crossed my mind, but was worried they'd be too long. Buuuut we're not going to be using the constructors of these models very often, so the cost of making them overly verbose is approx. 0 👍

I'll do that!

Copy link
Member

@phipsgabler phipsgabler left a comment

Choose a reason for hiding this comment

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

Aren't the implicitly constructed named tuples ((; m, x)) too new to be backwards compatible with 1.3?

src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@torfjelde
Copy link
Member Author

Is this ready to go, or is there something else that needs to be addressed?

meanfunction,
sampler::AbstractMCMC.AbstractSampler,
args...;
target=8.0,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, not entirely certain what you're saying 😅 It's "random" in the sense that it just happened to be the number that you get from the default values of the models, but it's deliberate in the sense that it's the actual true mean of the posterior of the models with the default values:)

Copy link
Member

Choose a reason for hiding this comment

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

So it's really the expected result, but only if you pass meanfunction = mean? I suspected it's either that or a random you put in for testing.

Copy link
Member

@phipsgabler phipsgabler Aug 23, 2021

Choose a reason for hiding this comment

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

So if IIUC, maybe that is better?

function test_sampler_demo_models(
    meanfunction,
    sampler::AbstractMCMC.AbstractSampler,
    args...;
    target,
    atol=1e-1,
    rtol=1e-3,
    kwargs...,
)
    ...
end
test_sampler_demo_models(::typeof(mean), args...; kwargs...,) = test_sampler_demo_models(mean, args...; target=8.0, kwargs...)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not valid just for mean though. This is because different samplers can have completely different return-values from sample, and so we want to allow different mean functions, while still wanting the target to be 8.0.

@phipsgabler
Copy link
Member

phipsgabler commented Aug 23, 2021

Apart from the magic 8.0, looks good to me.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Some additional suggestions 🙂

src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
torfjelde and others added 2 commits August 26, 2021 19:36
@torfjelde
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Sep 8, 2021
This PR adds a `DynamicPPL.TestUtils` submodule which is meant to include functionality to make it easy to test new samplers, new implementations of `AbstractVarInfo`, etc.

As of right now, this is mainly just a collection of models with equivalent marginal posteriors using the different features of DPPL, e.g. some are using `.~`, some are using `@submodel`, etc.

Eventually this should be expanded to be of more use, but more immediately this will be useful to test functionality in open PRs, e.g. #269, #309, #295, #292.

These models are also already used in Turing.jl's test-suite (https://github.com/TuringLang/Turing.jl/blob/9f52d75c25390b68115624b2e6cf464275a88137/test/test_utils/models.jl#L55-L56), so this PR would avoid the code-duplication + make it easier to keep things up-to-date.
@bors
Copy link
Contributor

bors bot commented Sep 8, 2021

Build failed:

@torfjelde
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Sep 9, 2021
This PR adds a `DynamicPPL.TestUtils` submodule which is meant to include functionality to make it easy to test new samplers, new implementations of `AbstractVarInfo`, etc.

As of right now, this is mainly just a collection of models with equivalent marginal posteriors using the different features of DPPL, e.g. some are using `.~`, some are using `@submodel`, etc.

Eventually this should be expanded to be of more use, but more immediately this will be useful to test functionality in open PRs, e.g. #269, #309, #295, #292.

These models are also already used in Turing.jl's test-suite (https://github.com/TuringLang/Turing.jl/blob/9f52d75c25390b68115624b2e6cf464275a88137/test/test_utils/models.jl#L55-L56), so this PR would avoid the code-duplication + make it easier to keep things up-to-date.
@bors
Copy link
Contributor

bors bot commented Sep 9, 2021

Build failed:

@torfjelde
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Sep 9, 2021
This PR adds a `DynamicPPL.TestUtils` submodule which is meant to include functionality to make it easy to test new samplers, new implementations of `AbstractVarInfo`, etc.

As of right now, this is mainly just a collection of models with equivalent marginal posteriors using the different features of DPPL, e.g. some are using `.~`, some are using `@submodel`, etc.

Eventually this should be expanded to be of more use, but more immediately this will be useful to test functionality in open PRs, e.g. #269, #309, #295, #292.

These models are also already used in Turing.jl's test-suite (https://github.com/TuringLang/Turing.jl/blob/9f52d75c25390b68115624b2e6cf464275a88137/test/test_utils/models.jl#L55-L56), so this PR would avoid the code-duplication + make it easier to keep things up-to-date.
@bors bors bot changed the title Add TestUtils submodule [Merged by Bors] - Add TestUtils submodule Sep 9, 2021
@bors bors bot closed this Sep 9, 2021
@bors bors bot deleted the tor/test-utils branch September 9, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants