-
Notifications
You must be signed in to change notification settings - Fork 37
Improvements to TestUtils (follow-up from #360) #415
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
Improvements to TestUtils (follow-up from #360) #415
Conversation
…stUtils and starting using this in test_continuous_models
|
@devmotion would you have a look at this so we can just merge this together with #360 ? This PR only makes changes to testing, and it would be nice to make sure that #360 passes with the additional tests provided in this PR before making a release anyways. |
src/test_utils.jl
Outdated
| # TODO: Is this really the best/most convenient "default" test method? | ||
| """ | ||
| test_sampler_demo_models(meanfunction, sampler, args...; kwargs...) | ||
| test_sampler_on_models(meanfunction, models, sampler, args...; kwargs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just
| test_sampler_on_models(meanfunction, models, sampler, args...; kwargs...) | |
| test_sampler(meanfunction, models, sampler, args...; kwargs...) |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other points:
- Since we test the
samplerbut not the models maybe that should be the second argument? - Is it useful to be able to specify a
meanfunction? Wouldn't it be sufficient if we provide defaults forChain,Vector{<:NamedTuple},Array{T,3}(or whatever common types are) asmarginal_means(chain). We would always usemarginal_meansbut users could still implementmarginal_meansfor custom types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we test the sampler but not the models maybe that should be the second argument?
I'm happy to make that change:) The reason why I did the current ordering is because it's similar to sample which has model first.
Is it useful to be able to specify a meanfunction? Wouldn't it be sufficient if we provide defaults for Chain, Vector{<:NamedTuple}, Array{T,3} (or whatever common types are) as marginal_means(chain). We would always use marginal_means but users could still implement marginal_means for custom types.
With the number of different Transition types we have lying around, I think it might be worth just allowing someone to quickly do
test_sampler(...) do chain, vn
# Compute whatever.
...
endBut happy to make it a separate function that can be overloaded or something too 👍
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
…ynamicPPL.jl into tor/test-utils-improvements
| Bijectors.logpdf_with_trans(d::NoDist{<:Univariate}, ::Real) = 0 | ||
| Bijectors.logpdf_with_trans(d::NoDist{<:Multivariate}, ::AbstractVector{<:Real}) = 0 | ||
| function Bijectors.logpdf_with_trans(d::NoDist{<:Multivariate}, x::AbstractMatrix{<:Real}) | ||
| Bijectors.logpdf_with_trans(d::NoDist{<:Univariate}, ::Real, ::Bool) = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that currently on #master anything involving NoDist + linked VarInfo is producing incorrect results (which, AFIAK, we were unaware of) 🙃
…hood_true methods
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Anything missing to merge this PR into #360? |
|
The only thing remaining is to resolve the current open comments in this PR, but these are very minor. I would recommend that we get #360 approved before we merge the two though, and then once that's approved we merge this into #360 . Then we leave it up to bors. This will reduce the amount of code you'll have to look at in one go when reviewing the PRs. |
|
I've made the requested changes here; should be good to go for merge once #360 has been approved. |
devmotion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (didn't recheck everything in detail).
When working on #360 I realized there were quite a few features we're not covering with the models provided in
DynamicPPL.TestUtils, e.g.linkandinvlinkare not covered since the current models do not include any constrained variables. This PR adds some additional methods inDynamicPPL.TestUtilswhich ought to be implemented for the models and a more generaltest_sampler_on_models.EDIT: No need to review this until we've merged #360 but I wanted some of these tests for
SimpleVarInfoto make sure it works properly.