v0.10 #305
Draft
v0.10 #305
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
JuliaBUGS.jl documentation for PR #305 is available at: |
Pull Request Test Coverage Report for Build 15559296011Details
💛 - Coveralls |
The scope of `interface.jl` is unclear—it contains a set of miscellaneous functions defined for models, but it's hard to see why they differ from `condition` and `evaluation`. Update: merged `interface.jl` into `model.jl` to balance code per file and keep related code together. --------- Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Xianda Sun <[email protected]>
|
I forgot in #307: |
1. `FlattenedGraphNodeData` is renamed to `GraphEvaluationData`. 2. `parameter` field of `BUGSModel` is moved inside `GraphEvaluationData` and named `sorted_parameters` for better understanding. (1) is purely for ease of understanding. (2) is motivated by eliminating an implicit invariant where `parameters` are assumed to be order-consistent with `sorted_nodes` field of `FlattenedGraphNodeData`, not explicit by construction.
Address #252 The old `condition` does two things at once: it creates a `BUGSModel` that contains a subset of the original variables and also mark variables to be conditioned on as `observed`. This was partially motivated by trying to make BUGS' `Gibbs` algorithm looks like Turing.jl's while make use the advantage of the graph. But this is ultimately a badly thought-out interface. This PR implements `condition` and `decondition` to be like DynamicPPL's, i.e., it only sets variables from model parameters to observations. I also took the chance to properly refactored the code to better handle more types of arguments and added some docstrings.
This fix some issues following the file restructuring. It's debatable `source_generation.jl` file should be moved to `model` folder. I left it as is. There is an error message removed. It is tricky - I think expose the raw error to user is probably a better idea, and we can improve the error message if it's required. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
make `/test` mimic `/src` structure-wise. one redundant test is removed, others was just moved around, some files are renamed. CI are split into parallel runs. `experimental` and some hmc tests are failing and I am going to ignore them for now. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There is a subtle incoherence when doing conditioning - the conditioned model inherent the base model's generated log density function. This PR addresses this and checks that the source gen works correctly for conditioned models.
# Gibbs Sampler Refactor This PR refactors the Gibbs sampler internals to fix correctness issues and improve performance. ## What Changed ### 1. Fixed MHFromPrior Bug (#250) - MHFromPrior now correctly samples from the posterior (not the prior) - Made MHFromPrior a proper `AbstractMCMC.AbstractSampler` that can be used standalone - Added `sample_all=false` to avoid resampling observed data ### 2. Performance Improvements - **Smart copying**: Only copy mutable parts of evaluation environments (5-35x faster) - **Direct state usage**: Gibbs now works directly with evaluation environments - Benchmark: 10-20x speedup for typical models with large datasets ### 3. Sampler Map Verification - Added automatic verification that all parameters are covered exactly once - Supports variable subsuming (e.g., `x` covers `x[1]`, `x[2]`, etc.) - Clear error messages for missing or duplicate parameters ### 4. AD Backend Selection - New `WithGradient` wrapper to specify AD backend per sampler: ```julia sampler_map = OrderedDict( @varname(μ) => WithGradient(HMC(0.1, 10), :ForwardDiff), @varname(σ) => WithGradient(NUTS(), :ReverseDiff) # default ) ``` - Backward compatible - unwrapped samplers default to ReverseDiff ### 5. Better State Management - Gibbs maintains sub-sampler states across iterations (preserves HMC/NUTS adaptation) - Parameter names preserved in chains (no more `:param_1`, `:param_2`) - Proper `bundle_samples` implementations for all samplers ## Testing - All existing tests pass - Added ~80 new tests covering correctness, performance, and edge cases - Verified against analytical posteriors (Normal-Normal, linear regression) - Mixed discrete/continuous parameter models tested ## Backward Compatibility - Public API unchanged - Existing Gibbs code continues to work - Only internal state structure changed ## Performance Example For a model with 30 data points and mixed continuous/discrete parameters: <details> <summary>model</summary> ```julia model_def = @bugs begin # Continuous parameters μ ~ Normal(0, 10) log_σ ~ Normal(0, 1) σ = exp(log_σ) # Discrete parameter k ~ Categorical(p[:]) # Likelihood for i in 1:N y[i] ~ Normal(μ + k * 0.5, σ) end end # Generate data Random.seed!(123) N = 30 p_data = [0.3, 0.4, 0.3] y_data = randn(N) .+ 1.5 model = compile(model_def, (; N=N, p=p_data, y=y_data)) ``` </details> the per-sample time and total memory comparison is | Configuration | main | This PR | |--------------|------|---------| | Mixed (HMC + MHFromPrior) | 37.5ms / 10.9GB | **2.12ms / 0.9GB** | | All MHFromPrior | 36.3ms / 9.8GB | **0.164ms / 0.05GB** | ## Files Changed - Core refactor: `src/gibbs.jl`, `src/mh_from_prior.jl` - Extensions updated: `ext/JuliaBUGS*Ext.jl` - Model improvements: `src/model/*.jl` - New tests: `test/gibbs.jl`, `test/mh_from_prior.jl` --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
The state of this PR: The most technical challenging part of this PR is complete. |
some better implemented and tested graph related code was added in #304. this PR removed the old code and refactor the tests.
Thanks @sunxd3! I've rebased the changes onto sunxd/v0.10 as requested and opened this new PR accordingly. Totally understand the ongoing breaking changes — happy to adjust anything here to better align with the direction of the branch. Let me know if there's anything you'd like revised or if it makes sense to hold off until things stabilize a bit more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to address #296. The changes required from current main branch will be substantial.
The current approach is that I'll gradually merge PR into
v0.10branch. And eventually make a non-squash merge.I am not entirely clear about the code change that are required, so I might flip-flop on ideas and design, but I'll make the motivation and changes clear.
I will likely to move fast and not request a lot of reviews, and sometimes code changes will break some tests, but their corresponding test will pass. And before merging , all tests will pass.
TODO: disabling
experimentaltest for now, revive later.