-
Notifications
You must be signed in to change notification settings - Fork 227
Gibbs test | Fix dynamic model test in Gibbs sampler suite #2579
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
…ce sampling validation
Leave this as a draft for a teeny bit, I need to properly test and fully understand how julia tests work so I'm not breaking what I think I understand - no need for any reviews yet. |
This should address: #2402 when completed. |
As discussed on Slack, feel free to change the base branch of this to |
Results from my local testing: Test Summary: | Pass Total Time
Turing | 2613 2613 19m50.9s
Aqua | 0 0.1s
AD | 0 0.0s
essential | 0 0.0s
samplers (without AD) | 0 0.0s
inference with samplers | 2613 2613 19m50.8s
GibbsContext | 868 868 27.7s
type stability | 868 868 27.7s
Invalid Gibbs constructor | 8 8 0.1s
Sampler call order | 1 1 53.9s
Equivalence of RepeatSampler and repeating Sampler | 1 1 4.5s
Gibbs warmup | 8 8 2.3s
Testing gibbs.jl | 1727 1727 18m19.3s
Gibbs constructors | 13 13 19.7s
Gibbs inference | 22 22 6m06.7s
transitions | 0 2.1s
dynamic model with analytical posterior | 5 5 8.4s
dynamic model with ESS | 6 6 33.4s
dynamic model with dot tilde | 0 11.3s
Demo model | 1564 1564 8m32.2s
multiple varnames | 20 20 1.5s
non-identity varnames | 2 2 9.2s
submodels | 2 2 11.3s
CSMC + ESS | 42 42 38.9s
CSMC + ESS (usage of implicit varname) | 42 42 51.8s
externalsampler | 8 8 28.2s
linking changes dimension | 1 1 4.5s
variational algorithms | 0 0.0s
mode estimation | 0 0.0s
variational optimisers | 0 0.0s
stdlib | 0 0.0s
utilities | 0 0.0s
───────────────────────────────────────────────────────────
Time Allocations
─────────────── ───────────────
Total measured: 1191s 534GiB
Section ncalls time %tot alloc %tot
───────────────────────────────────────────────────────────
inference 1 1191s 100.0% 534GiB 100.0%
mcmc/gibbs.jl 1 1191s 100.0% 534GiB 100.0%
─────────────────────────────────────────────────────────── Testing Turing tests passed |
Turing.jl documentation for PR #2579 is available at: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2579 +/- ##
=======================================
Coverage 85.57% 85.58%
=======================================
Files 22 22
Lines 1456 1457 +1
=======================================
+ Hits 1246 1247 +1
Misses 210 210 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 15822411978Details
💛 - Coveralls |
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.
Pull Request Overview
This pull request refactors several model definitions and tests to adopt the new default-type parameter syntax and introduces a new dynamic model (dynamic_bernoulli_normal) with an analytical posterior for faster and more robust testing of the Gibbs sampler. Key changes include:
- Updating function signatures across multiple tests to use the new parameter syntax with parentheses.
- Introducing a dynamic model with a Bernoulli variable controlling parameter dimensionality.
- Simplifying and speeding up tests while ensuring core Gibbs sampling functionality is exercised.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/mcmc/hmc.jl | Updated the default type parameter syntax in model definitions. |
test/mcmc/gibbs.jl | Updated function signatures and added a new dynamic model test. |
test/mcmc/Inference.jl | Applied similar parameter signature updates across model tests. |
src/mcmc/external_sampler.jl | Updated constructor and helper function parameter syntax. |
Comments suppressed due to low confidence (2)
test/mcmc/gibbs.jl:564
- [nitpick] The check for θ[2] values only verifies that one of the conditions is met. Consider splitting the test into two separate assertions to clearly validate that when b=0, θ[2] is missing, and when b=1, θ[2] exists.
@test n_missing_theta2 > 0 || n_present_theta2 > 0 # One of these should be true
src/mcmc/external_sampler.jl:47
- [nitpick] Consider adding a comment or type annotations to clarify the expected type and meaning of the 'unconstrained' flag, which would improve maintainability and readability of the code.
function requires_unconstrained_space(
Thanks @AoifeHughes -- good work! |
# Helper function for logsumexp | ||
function logsumexp(x) | ||
max_x = maximum(x) | ||
return max_x + log(sum(exp.(x .- max_x))) | ||
end |
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.
Minor, but it might be better to use LogExpFunctions.logsumexp
here rather than rolling our own version.
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.
yes
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.
Otherwise, if @yebai if happy, I am too
The existing dynamic model test was problematic:
Here I've changed this with:
a new dynamic_bernoulli_normal model that: