Convert course code into MFIIDD package (draft for #22)#39
Merged
Conversation
sbfnk-bot
changed the title
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
…lay the real source Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
…GitHub Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
…, gloss the SSM interface block Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
sbfnk
force-pushed
the
spike/package-mfiidd
branch
from
1fc41a2 to
cf7e429
Compare
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
The 2.x release changed the __initial_guess_on_mesh signature and is incompatible with BoundaryValueDiffEqFIRK 1.13. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
NUTS uses ForwardDiff Dual numbers for the parameter vector. Without promoting u0, the solver tried to write Duals into a Float64 vector and crashed with a Float64 conversion error. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Turing's externalsampler init probe wraps parameters in ForwardDiff Duals to check the gradient is finite, which fails inside GeneralisedFilters because the bootstrap filter is non-differentiable. Apply ForwardDiff.value on each prior draw before building the SSM, so the init probe and the actual MH steps both see Float64 parameters. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
NUTS occasionally probes parameter combinations where Tsit5 only saves the initial timepoint, leaving inc_daily one element long and tripping a BoundsError in the lambdas comprehension. Check the retcode and saved-points length, addlogprob! -Inf to discard the draw if either fails. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Turing 0.44 wraps rand(model) results in a VarNamedTuple; the inner NamedTuple lives at .data, so direct field access on the wrapper now errors. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Spike's wider compat bounds let the resolver pick newer Lux/OptimizationBase combinations whose chunked ForwardDiff path errors with PartialsFn(::Float64) on ComponentArray. Match main's working pin until upstream sorts the interaction out. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Without the promotion, the ODE solve drops ForwardDiff Duals, the loss returns Float64, and ForwardDiff's chunked gradient extraction trips a Lux/ComponentArrays edge case (PartialsFn called on a Float64). This matches the fix used for the deterministic SEIT4L model. Drops the earlier Lux/OptimizationBase pins now that the underlying issue is addressed. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
q_meanfield_gaussian/q_fullrank_gaussian now act as families passed to vi, rather than being called on the model up front. vi returns a VIResult; the optimised distribution is res.q and the training stats res.info. Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors the codebase into a Julia package module ChangesModule refactor & runtime/autodiff changes
Quarto include-code-files extension
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@_extensions/quarto-ext/include-code-files/include-code-files.lua`:
- Around line 7-9: dedent currently strips all spaces within the first n
characters which mutates tokens; change it so it only removes up to n leading
spaces (i.e., compute how many leading spaces the line actually has, take min(n,
leadingCount), and remove that many characters from the start) and then return
the reconstructed line. Update the dedent(line, n) implementation to detect
leading whitespace (using line:match or equivalent), subtract only that many
leading spaces (not internal spaces in the first n chars), and return the
trimmed line.
In `@src/seit4l_bootstrap_filter.jl`:
- Around line 19-25: The particle_filter_seit4l function lacks a guard against
non-positive n_particles which leads to runtime errors later (e.g.,
maximum(log_w)); add an upfront check in particle_filter_seit4l that verifies
n_particles is an integer > 0 and throw a clear ArgumentError (or return/handle
as appropriate) if the check fails, so downstream code that assumes a non-empty
particle set (referencing n_particles, particles, and maximum(log_w)) never runs
with zero or negative particles.
In `@src/seit4l_gillespie.jl`:
- Around line 44-47: The simulation loop in gillespie_step_seit4l! and its
wrapper ignores the dt parameter by using a hardcoded horizon (t < 1.0), so
non-daily intervals are never honored; change the loop condition(s) from t < 1.0
to t < dt, ensure any wrapper functions that call gillespie_step_seit4l! pass
their dt argument through (or add a default dt and forward it), and update all
occurrences referenced in the diff (the loop in gillespie_step_seit4l! and the
wrapper calls around lines 53 and 83–85) so the exposed dt actually controls how
far the while loop advances. Ensure tests or callers that relied on the 1.0
behavior are adjusted accordingly.
In `@src/seit4l_runner.jl`:
- Around line 22-23: The constructors SEIT4LDynamics and PoissonObservation
expect concrete Float64 fields but are being passed θ that may contain
ForwardDiff.Duals; before creating dynamics and observation, convert θ to plain
Float64 values (e.g. use ForwardDiff.value or ForwardDiff.value. on θ or
reconstruct a Float64-typed parameter NamedTuple) and then pass that normalized
θ to SEIT4LDynamics(...) and PoissonObservation(...), mirroring the pattern used
in pmcmc.qmd so Duals do not flow into the particle filter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1384894-3c7a-4d4e-b0f9-1bb3893cf9fd
📒 Files selected for processing (24)
Manifest.tomlProject.toml_extensions/quarto-ext/include-code-files/_extension.yml_extensions/quarto-ext/include-code-files/include-code-files.lua_quarto.ymlscripts/generate_pmcmc_chains.jlscripts/generate_pmcmc_seit4l.jlscripts/generate_pmcmc_seitl.jlsessions/mcmc.qmdsessions/mcmc_diagnostics.qmdsessions/particle_filters.qmdsessions/pmcmc.qmdsessions/seitl.qmdsessions/universal_differential_equations.qmdsessions/variational_inference.qmdsrc/MFIIDD.jlsrc/seit4l_bootstrap_filter.jlsrc/seit4l_gillespie.jlsrc/seit4l_particle_filter.jlsrc/seit4l_runner.jlsrc/seit4l_ssm_interface.jlsrc/seitl_model.jlsrc/seitl_particle_filter.jlsrc/sir_model.jl
💤 Files with no reviewable changes (4)
- src/seitl_model.jl
- src/seitl_particle_filter.jl
- src/sir_model.jl
- src/seit4l_particle_filter.jl
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
…n_particle_filter Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Owner
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sessions/pmcmc.qmd (1)
176-195: ⚡ Quick winUse
run_particle_filterhere instead of re-implementing it.This block now duplicates
src/seit4l_runner.jl, so fixes to the packaged helper can drift away from the PMMH session. Given the new package structure, I'd keep the explanatory callout above for pedagogy but delegate the actual likelihood evaluation torun_particle_filter(...).Suggested simplification
- # Create parameter dictionary; extract underlying values so ForwardDiff - # Duals (used only by Turing's gradient-based init probe) don't flow into - # the particle filter, which is non-differentiable anyway. - θ = Dict(:R_0 => ForwardDiff.value(R_0), - :D_lat => ForwardDiff.value(D_lat), - :D_inf => ForwardDiff.value(D_inf), - :α => ForwardDiff.value(α), - :D_imm => ForwardDiff.value(D_imm), - :ρ => ForwardDiff.value(ρ)) - - # Build state-space model - # The SSMProblems interface expects a plain vector, not a Dict - # Order: [S, E, I, T1, T2, T3, T4, L] - init_state = [279.0, 0.0, 2.0, 3.0, 0.0, 0.0, 0.0, 0.0] - ssm = StateSpaceModel(SEIT4LInitial(init_state), - SEIT4LDynamics(θ), - PoissonObservation(θ[:ρ])) - - # Run particle filter to estimate log-likelihood - _, log_lik = GeneralisedFilters.filter(Random.default_rng(), ssm, BF(n_particles), obs) + θ = Dict(:R_0 => R_0, + :D_lat => D_lat, + :D_inf => D_inf, + :α => α, + :D_imm => D_imm, + :ρ => ρ) + + init_state = [279.0, 0.0, 2.0, 3.0, 0.0, 0.0, 0.0, 0.0] + log_lik = run_particle_filter(θ, obs, n_particles; init_state=init_state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sessions/pmcmc.qmd` around lines 176 - 195, The code duplicates the particle-filter call; replace the manual construction and GeneralisedFilters.filter call with the packaged helper run_particle_filter so the session uses the canonical implementation: build θ (using ForwardDiff.value) and the same init_state/SSM (SEIT4LInitial, SEIT4LDynamics, PoissonObservation) as before but call run_particle_filter(ssm, obs; n_particles = n_particles, rng = Random.default_rng()) (or the exact run_particle_filter signature in src/seit4l_runner.jl) and capture the returned log-likelihood instead of calling GeneralisedFilters.filter/BF directly; ensure you pass the same n_particles and rng options to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/seit4l_runner.jl`:
- Around line 19-29: run_particle_filter currently passes init_state directly
into SEIT4LInitial which expects Vector{Float64}; coerce and validate init_state
inside run_particle_filter before creating initial: convert to a Vector{Float64}
(e.g. use Float64.(collect(init_state)) or similar) and check its length matches
the expected size (8) and elements are finite, then pass the coerced vector to
SEIT4LInitial; update the function where SEIT4LInitial is constructed (in
run_particle_filter) and ensure the default init_state remains compatible by
normalizing it the same way.
---
Nitpick comments:
In `@sessions/pmcmc.qmd`:
- Around line 176-195: The code duplicates the particle-filter call; replace the
manual construction and GeneralisedFilters.filter call with the packaged helper
run_particle_filter so the session uses the canonical implementation: build θ
(using ForwardDiff.value) and the same init_state/SSM (SEIT4LInitial,
SEIT4LDynamics, PoissonObservation) as before but call run_particle_filter(ssm,
obs; n_particles = n_particles, rng = Random.default_rng()) (or the exact
run_particle_filter signature in src/seit4l_runner.jl) and capture the returned
log-likelihood instead of calling GeneralisedFilters.filter/BF directly; ensure
you pass the same n_particles and rng options to preserve behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4f97a09-36ba-4cca-918f-4d8e175b65a5
📒 Files selected for processing (11)
_extensions/quarto-ext/include-code-files/include-code-files.luasessions/mcmc.qmdsessions/mcmc_diagnostics.qmdsessions/particle_filters.qmdsessions/pmcmc.qmdsessions/seitl.qmdsessions/universal_differential_equations.qmdsessions/variational_inference.qmdsrc/seit4l_bootstrap_filter.jlsrc/seit4l_gillespie.jlsrc/seit4l_runner.jl
✅ Files skipped from review due to trivial changes (3)
- sessions/universal_differential_equations.qmd
- _extensions/quarto-ext/include-code-files/include-code-files.lua
- sessions/particle_filters.qmd
🚧 Files skipped from review as they are similar to previous changes (5)
- src/seit4l_gillespie.jl
- sessions/mcmc_diagnostics.qmd
- src/seit4l_bootstrap_filter.jl
- sessions/variational_inference.qmd
- sessions/seitl.qmd
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Owner
|
@seabbs can't tag you for review (yet) but would be good if you could have a look. |
seabbs
reviewed
May 5, 2026
seabbs
reviewed
May 5, 2026
seabbs
reviewed
May 5, 2026
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
This was referenced May 11, 2026
3 tasks
Collaborator
Author
|
Wait-for-review summary:
This watcher is stopping; auto-merge not queued because the latest review state is |
docs/Project.toml + docs/make.jl + docs/src/{index,api}.md generate an
@autodocs API reference for the MFIIDD module under /api. Run locally
with:
julia --project=docs -e 'using Pkg; Pkg.develop(path="."); Pkg.instantiate(); include("docs/make.jl")'
Deployment to gh-pages is not wired up in CI yet — that belongs in
.github/workflows and is outside the scope of this PR.
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Sessions previously inlined files via {.julia include="../src/X.jl"},
which hardcodes a path relative to the qmd. source_for reads the file
from the live pkgdir(MFIIDD) instead, and also accepts a function to
print just its methods via CodeTracking.
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
Co-authored-by: sbfnk <sebastian.funk@lshtm.ac.uk>
sbfnk
approved these changes
Jun 29, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Draft spike for #22.
Turns the project into a Julia package called
MFIIDD. Splitssrc/seit4l_particle_filter.jlinto focused files. Sessions and chain-generation scripts now dousing MFIIDDinstead of path-jugglinginclude(...). Adds thequarto-ext/include-code-filesextension so source-display blocks pull from the actual src files (which incidentally fixes a bug where the previous#| eval: false #| file:callouts inmcmc.qmd,mcmc_diagnostics.qmd,pmcmc.qmd, andparticle_filters.qmdhad been silently rendering empty for months).What this addresses:
PrecompileToolsworkload inMFIIDD.jl— the main argument from the Make code a package #22 threadinclude(joinpath(@__DIR__, "..", "src", ...))path juggling in sessions and scriptssrc/*.jlfile, no copy-paste driftsrc/...jlmention in the session proseWhat this leaves alone:
MFIIDDis consumed in-tree viaPkg.activate(".")[compat] julia = "1.12"matches the CI pinNotes for review:
particle_filters.qmdusing MFIIDDline to be framed as the course package — done#= ... =#file-header comments (some referencing the deletedseit4l_particle_filter.jlor the obsoleteincludepattern) were leaking into the rendered output; strippedjuliaup override set 1.12.6in the project directory pins the Julia version locally;[compat] juliaenforces it at instantiate timePlease review with a focus on whether you'd actually want this merged, or whether the student-shortcut concern still outweighs the package benefits in your view.
Summary by CodeRabbit
New Features
Improvements
Documentation