Skip to content

Add Bayesian posterior sampling to Analysis and ParameterAnalysis - #238

Open
henrikjacobsenfys wants to merge 11 commits into
bayesianfrom
bayesian-analysis
Open

Add Bayesian posterior sampling to Analysis and ParameterAnalysis#238
henrikjacobsenfys wants to merge 11 commits into
bayesianfrom
bayesian-analysis

Conversation

@henrikjacobsenfys

Copy link
Copy Markdown
Member

Extends the Bayesian sampling from #237 to the remaining two Analysis classes.

Stacked on #237 — please merge that first; this PR targets the bayesian branch, and I will retarget it at develop once #237 lands.

No new sampling machinery. Each class supplies the three mixin hooks — build the fitter, bind the data, list the chain parameters — and everything else is shared, which was the point of building #237 mixin-first.

Analysis

sample_posterior(fit_method=...) mirrors fit():

# one chain per Q index, delegating to the Analysis1d objects
results = analysis.sample_posterior(fit_method='independent')      # list, one per Q
result  = analysis.sample_posterior(fit_method='independent', Q_index=5)

# one chain over every Q at once, through a MultiFitter
result  = analysis.sample_posterior(fit_method='simultaneous')

The simultaneous path refreshes each per-Q convolver against its masked energy grid before sampling, exactly as _fit_all_Q_simultaneously does, so the sampler sees the model evaluations the fit would have made. This was the open question at the end of #237, and it works: 9 parameters across 3 Q values sample in ~14s.

ParameterAnalysis

Samples the binding models. fit() built its MultiFitter inline, so the per-target data, functions, and models are now resolved by a shared _build_fit_inputs() used by both paths — which also guarantees fitting and sampling see the same targets, in the same order, with the same unit conversions.

Parameter labels

This needed rethinking for multi-Q. A multi-Q analysis holds one copy of each parameter per Q, all sharing a name — three parameters called Gaussian width. A summary showed three identical rows, and a name could not be used to select a parameter.

Labels now come from an overridable parameter_label(): Analysis qualifies by Q index, ParameterAnalysis by binding model, and both only when the bare name is actually ambiguous, so single-Q and single-binding cases keep their short names. The summary and bounds tables size themselves to the longest label instead of truncating at a fixed width.

Drive-by fix

Analysis.fit's docstring promised "a single FitResults object if fitting simultaneously". MultiFitter splits its combined result back up per dataset, so a list has always been returned. Docstring corrected to match the behaviour.

Tutorial

Tutorial 1 gains a Bayesian section on the two-step diffusion fit, and it surfaces something worth documenting: the posterior comes out about twelve times tighter than the reported least-squares uncertainties.

That gap is real, not a bug. The width fit has a reduced χ² of ~150, so lmfit inflates its reported uncertainties by √(reduced χ²) ≈ 12 on the assumption that a poor fit means understated input uncertainties. DREAM makes no such adjustment. Neither is simply right, and the gap is itself a signal that the two-step model is not capturing the data — which is exactly what the tutorial goes on to address by fitting everything simultaneously.

I originally intended to sample the full simultaneous diffusion model, but measured it at over ten minutes — too slow for a tutorial and for CI. The ParameterAnalysis step gives the same lesson in ~25s, and tutorial 1 as a whole runs in 41s.

Testing

  • 41 new tests; 1630 pass in total.
  • Unit tests mock the sampler. Integration tests run real chains (~55s) covering simultaneous multi-Q, independent-vs-simultaneous agreement, and ParameterAnalysis.
  • One integration test doubles as a check that suggest_bounds refuses to invent a scale: a polynomial coefficient sitting at exactly zero with vanishing uncertainty is flagged rather than bounded, and absolute_floor is what resolves it.
  • pixi run fix clean; pixi run check clean except a pre-existing CONTRIBUTING.md prettier difference that also fails on a clean tree at HEAD (local npx prettier is newer than CI's — CI passes).

🤖 Generated with Claude Code

Extends the sampling introduced for Analysis1d to the remaining two
Analysis classes, using the mixin hooks added with it. No new sampling
machinery: each class supplies its fitter, its data, and its chain
parameters, and everything else is shared.

Analysis gains sample_posterior(fit_method=...), mirroring fit():

- 'independent' gives each Q index its own chain, delegating to the
  Analysis1d objects, and returns one result per Q (or a single result
  when a Q_index is given).
- 'simultaneous' runs one chain over every Q at once through a
  MultiFitter, refreshing each per-Q convolver against its masked energy
  grid first, exactly as the simultaneous fit does.

ParameterAnalysis samples the binding models. Its fit() built the
MultiFitter inline, so the per-target data, functions, and models are
now resolved by a shared _build_fit_inputs() that both paths use, which
also guarantees fitting and sampling see the same targets in the same
order with the same unit conversions.

Parameter labels needed rethinking. A multi-Q analysis holds one copy of
each parameter per Q, all sharing a name, so a summary showed several
identical rows and a name could not pick a parameter out. Labels are now
produced by an overridable parameter_label(): Analysis qualifies by Q
index, ParameterAnalysis by binding model, and both only when the bare
name is actually ambiguous, so single-Q and single-binding cases keep
their short names. The summary and bounds tables size themselves to the
longest label rather than truncating.

Also fixes Analysis.fit's docstring, which promised a single FitResults
for a simultaneous fit. MultiFitter splits its combined result back up
by dataset, so a list has always been returned.

Tutorial 1 gains a Bayesian section on the two-step diffusion fit, where
the posterior turns out to be about twelve times tighter than the
reported least-squares uncertainties. That gap is real and worth
explaining: the width fit has a reduced chi-squared near 150, so lmfit
inflates its uncertainties by the square root of that, while the sampler
takes the stated uncertainties at face value. Sampling the full
simultaneous diffusion model was measured at over ten minutes, so the
tutorial uses the ParameterAnalysis step instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@henrikjacobsenfys henrikjacobsenfys added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority labels Aug 13, 2026
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.34641% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.10%. Comparing base (a8946ce) to head (442a5bd).

Files with missing lines Patch % Lines
src/easydynamics/utils/posterior_plotting.py 96.15% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           bayesian     #238      +/-   ##
============================================
+ Coverage     98.47%   99.10%   +0.63%     
============================================
  Files            56       56              
  Lines          4533     4825     +292     
  Branches        774      831      +57     
============================================
+ Hits           4464     4782     +318     
+ Misses           36       18      -18     
+ Partials         33       25       -8     
Flag Coverage Δ
unittests 99.10% <99.34%> (+0.63%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/easydynamics/analysis/analysis.py 97.70% <100.00%> (+1.29%) ⬆️
src/easydynamics/analysis/bayesian_sampling.py 100.00% <100.00%> (+8.07%) ⬆️
src/easydynamics/analysis/parameter_analysis.py 99.59% <100.00%> (+0.08%) ⬆️
src/easydynamics/analysis/posterior.py 98.78% <100.00%> (+2.18%) ⬆️
src/easydynamics/utils/posterior_plotting.py 96.92% <96.15%> (+4.51%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

henrikjacobsenfys and others added 10 commits August 13, 2026 13:48
The summary table already reported each parameter's unit, but the plots
did not, so a diffusion coefficient came out as a bare number. Units are
now threaded through to plot_trace and plot_corner, and the posterior
predictive plot gets axis labels taken from the analysis' own energy and
intensity units.

Details that needed care:

- Matplotlib parks a shared exponent at the end of the axis, on top of
  the axis label. It is now folded into the label, sharing one set of
  parentheses with the unit, so a diffusion coefficient reads
  "diffusion_coefficient (1e-8 m^2/s)" rather than stacking two
  parentheticals or overlapping.
- Dimensionless and empty units are skipped. A polynomial coefficient
  labelled "dimensionless" is noise.
- The top-left panel of a corner plot is a histogram, so its vertical
  axis counts draws rather than carrying a parameter. It is now labelled
  "counts" instead of being left blank, which read as an omission.
- Corner tick counts are capped, since four labelled ticks per panel is
  as much as a small panel can carry legibly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fixes found by writing the tests codecov asked for.

ParameterAnalysis qualified an ambiguous parameter with the owning
model's display_name, but for several models -- the diffusion models
among them -- display_name is the class name, so two models constructed
as name='Diffusion A' and name='Diffusion B' both came back as
"BrownianTranslationalDiffusion" and the label did not disambiguate
anything. It now uses the model's name, matching the choice to report
parameters under their name rather than their display name, and falls
back to the unique name only when the names collide too.

The rest is test coverage for branches that were reachable but untested:
the label fallbacks, the BUMPS outlier crash being re-raised as a
degeneracy hint, a chain column that matches no parameter, loading a
chain through its sidecar, the mixin's unimplemented hooks, and the
scientific-notation exponent being folded into an axis label.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The notebook tests run with '-n auto', and five of the notebooks fetch
vanadium_data_example.h5 through pooch. On a cold cache the workers race:
one is still writing the file into the cache while another opens it,
which fails on Windows with "PermissionError: Permission denied". This
failed twice in a row on windows-latest, always on that file, always
with the other sixteen notebooks passing.

The race is pre-existing, but adding a fifth notebook that wants the same
file, and lengthening tutorial 1, made it reliable rather than rare.

Fetching every tutorial data file once, before the parallel run starts,
leaves the workers with nothing to do but read, which is safe. The
prefetch reads the URLs and hashes out of the notebooks themselves, so it
cannot drift from what they actually download, and it never fails the
run: a file it cannot fetch is left to the notebook that needs it, which
reports the problem with far more context.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tegration tests

Two problems found while reviewing the previous commits.

Caching the MultiFitter on ParameterAnalysis introduced a regression. A
FitBinding can be edited in place -- binding.targets = ... -- which
ParameterAnalysis cannot observe. Changing the number of targets left the
cached fitter holding one fit function against two datasets, and fit()
died with "FitError: list index out of range". It rebuilt every call
before, so this worked previously. The targets the fitter was built for
are now recorded and compared, which is enough to catch an edit that
cannot be observed directly.

The integration tests then failed in CI on macOS, inside BUMPS' outlier
removal, on an identifiable model. That matters beyond the test: the
error message claimed the crash means degenerate parameters, and this
shows short chains do it too. The message now names both causes, and the
integration tests switch the outlier removal off, as they already do for
the burn-point trimming.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six issues found reviewing the previous commits.

The sidecar could be written with the wrong labels. A subset run built
the name map inside the block that holds the other parameters fixed,
where nothing looks ambiguous, so a multi-Q chain recorded unqualified
names that no longer matched on reload. The map is now built outside that
block, where the free set is the user's real one.

extend_sampling() accepted a different parameter subset. BUMPS resumes
from a stored chain whose width is fixed, so that could only fail deep
inside the sampler; it is now refused up front.

The IndexError relabelling was unconditional, so an IndexError from this
package would have been reported as a BUMPS modelling problem. It now
only applies when the traceback passes through bumps.

Labelling a chain was quadratic in the parameter count: collecting the
parameters and scanning for their owner both happened per parameter, and
each walks every sub-model. 75 parameters took 0.39 s, and every summary
and plot pays it. The parameters are now collected once per pass, and
Analysis keeps an owner index alongside its analysis list. The same case
now measures at 0.00 s.

Asking an Analysis for a summary after sampling independently reported
that nothing had been sampled, moments after it had. It now says where
the chains actually are.

Applying bounds many orders of magnitude wider than the parameter is
still allowed -- it is what the fit implied -- but no longer silent, so a
scripted apply() cannot hide a degeneracy the table would have shown.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three lines the review fixes added were not reachable from the unit
tests. Two are now covered: extending after a run that died before
storing results, where the chain-shape guard has nothing to compare
against, and a parameter shared across every Q index, which is left out
of the owner map because no single Q identifies it.

The third was the non-finite check in the absurd-width test, and it was
redundant rather than untested: an infinite width already compares
greater than any threshold, and the zero-scale case returns before it.
Removed, so the behaviour is unchanged and there is no dead branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sampling with fit_method='independent' left the results only on the
Analysis1d objects, so the Analysis that produced them could not report
on them. It now gathers them, but only where gathering is sound.

posterior_summary() collects every Q into one table, labelled by Q index,
and set_parameters_to_posterior_median() applies each chain to its own Q.
Both are per-parameter marginal operations, and a marginal is well
defined within its own chain, so combining them across separate chains
says nothing that was not sampled.

plot_corner() deliberately does not aggregate. Independent sampling draws
each Q separately, so no draw pairs a parameter at one Q with a parameter
at another, and a corner plot built from them would show correlations
that are an artefact of how the sampling was run rather than anything
measured. It says so and points at the per-Q corner plots, which are
real. plot_trace() likewise, the chains being separate runs of different
lengths rather than one trace.

posterior_results exposes the per-Q chains directly, and a simultaneous
chain still takes precedence over stale per-Q ones.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent chains share no draws, so there is no joint distribution
across Q to plot, and combining them would show correlations that came
from how the sampling was run rather than from the data. Refusing
outright was correct but unhelpful: the correlations within each Q are
real and worth looking at.

Analysis.plot_corner() now shows one Q at a time. Pass Q_index for a
particular one, or leave it out in a notebook for a slider across the Q
values that were sampled. A simultaneous chain is unaffected; it already
covers every Q in one figure. Outside a notebook the error names the
sampled Q indices rather than only saying no.

The slider is built with append_display_data rather than the Output
widget's context manager. The context manager is the obvious choice and
captures nothing under some kernels, which would have shipped a slider
with a permanently blank panel beside it. Verified by executing a
notebook against a real kernel, and the test asserts the panel actually
holds a figure, since an empty panel is the regression that matters.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The slider was described in the tutorial's caveats but never
demonstrated: every notebook call to plot_corner() went through the
single-chain path, because the Bayesian tutorial used Analysis1d and
tutorial 1 used ParameterAnalysis, neither of which has a Q dimension.
So the only things exercising it were the unit tests.

The tutorial now builds the full multi-Q Analysis, samples a few Q
values, gathers them with posterior_summary(), and shows the slider.

It samples Q indices 4, 8 and 12 rather than all sixteen. Sampling every
Q measured at 70 s against 16 s for three, and the subset also shows two
things worth showing: that sampling is slow enough to be worth trying a
few Q values first, and that the slider offers only the Q values that
were actually sampled.

Verified against a real kernel that the cell emits a widget view, rather
than only that the notebook ran without raising.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches where plopp puts its slicer controls, which is also where the
existing slicerplot_with_residuals puts them via the figure's bottom bar.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[priority] medium Normal/default priority [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant