Reach the whole library through one namespace - #241
Merged
Conversation
Expose the EasyScience Fitter on Analysis1d and add MCMC posterior sampling on top of it, using the BUMPS DREAM sampler introduced in easyscience 2.5.1 (easyscience.fitting.Sampler). Least-squares fitting reports a single point with a curvature-derived uncertainty, which is only trustworthy when parameters are uncorrelated and roughly Gaussian. Sampling maps the whole posterior instead, so correlated and skewed parameters get honest credible intervals. The sampling machinery lives in a mixin with three hooks (build the fitter, bind the data, list the chain parameters) so that Analysis and ParameterAnalysis can reuse it. ParameterAnalysis is not an AnalysisBase and builds a MultiFitter over binding models rather than over itself, so a shared base class would not have worked. Notable details: - fit() now uses a cached Fitter instead of building one per call, and the cache is invalidated through the existing dirty-flag pattern. - Bounds are the prior in DREAM, so sampling refuses to run with any infinite bound. suggest_bounds() proposes finite ones from the fitted values and uncertainties; it is advisory until .apply() is called and never loosens a bound that is already finite, so physical limits survive. A zero-width suggestion is flagged rather than invented. - Sampling restores parameter values afterwards, since BUMPS leaves them wherever the last likelihood evaluation put them. - Chains are reported under Parameter.name, not the internal unique_name. Those names are per-session, so save_chain() writes a sidecar mapping them to stable names and load_chain() uses it; loading without one warns rather than mislabelling the columns. - After sampling, a warning fires when the posterior has piled up against a bound, which catches both bounds that are too tight and degenerate parameters that drift until a bound stops them. - BUMPS crashes with a bare IndexError inside its own outlier removal when chains scatter, which in practice means a degenerate model. That is re-raised with the likely cause and a workaround. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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>
Review feedback: the import style was inconsistent enough that a reader
had to scroll back to the imports cell to find out where a name came
from. Surveying it, the tutorials used four styles, and the last two
existed only because there was no other way to reach those names:
import easydynamics as edyn 32 uses
import easydynamics.sample_model as sm 151 uses
from easydynamics.convolution import Convolution forced
from easydynamics.utils.utils import hbar forced
easydynamics.__all__ held six names, so Analysis1d, Convolution,
detailed_balance_factor and hbar could only be had by importing the
module that defines them. The inconsistency was structural rather than
careless, and no amount of tidying the notebooks alone would have fixed
it.
Everything public is now re-exported from easydynamics, 37 names, so
`import easydynamics as edyn` reaches all of it. The sub-packages stay
importable and the internal layout is untouched: only the front door is
flat. Flat is comfortable at this size, there were no name collisions,
and the sample_model grouping was already imprecise, holding
InstrumentModel, ResolutionModel and BackgroundModel.
The tutorials and the docstring examples that render into the API
reference now use that one style throughout. A test keeps the front door
in step with the sub-packages and the notebooks in step with the
convention, which is also written down in CONTRIBUTING.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: bayesian_sampling.py had a lot in it that belonged elsewhere, and it was unclear why it was a mixin at all. It was a mixin because ParameterAnalysis is not an AnalysisBase and fits its binding models rather than itself, so a shared base class does not work. That was a reason, not a good one: it injected some forty methods into every Analysis class. The sampler is now composed. An Analysis exposes one `bayesian` property, and hands the sampler the few things that differ between the Analysis classes -- the data, the free parameters, their labels, and a hook to refresh cached computation -- so PosteriorSampler needs no knowledge of how any Analysis is built, and no Analysis inherits sampling machinery it does not use. Labelling moves to posterior_labels.py. Building it once for a fixed set of parameters also removes the quadratic cost the old code needed a scoped cache to avoid: the counts and lookups are computed in the constructor rather than per column. Plotting stays in posterior_plotting.py, where it already lived. The sampler keeps three short delegates so a chain can still be plotted from the object holding it, but none of the drawing happens there. The public API becomes analysis.bayesian.sample() and friends, and the explicit suggest_bounds().apply() step stays: in DREAM the bounds are the prior, and an unbounded parameter gives a confident-looking interval set by nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the BayesianSamplingMixin with a PosteriorSampler that each analysis holds, so sampling, labelling and plotting stop sharing one class. Analysis gets a MultiQPosteriorSampler on top, which keeps the per-Q chains and the Q slider. Labels move to ParameterLabels, built once per call rather than once per parameter, and each analysis supplies the qualifier it needs: Q index for Analysis, binding model name for ParameterAnalysis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The section headers still pointed at a class that no longer exists, and MultiQPosteriorSampler was reachable only through Analysis.bayesian. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flat namespace still re-exported the mixin that the refactor removed, and not the sampler classes that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prettier 3.9, which CI installs, measures the shield emoji differently from the older release cached here and wants the line whole. 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> (cherry picked from commit 46d745a)
The sampling tests labelled the action WHEN and had no THEN, so a reader could not see where the arrangement stopped and the call under test began. Setup is WHEN, the action is THEN, the assertions are EXPECT, and steps that genuinely collapse onto one statement carry one combined marker instead. Comments only; no test changed what it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same pass as on the single-Q tests: setup is WHEN, the action is THEN, the assertions are EXPECT, and a step that collapses onto one statement carries one combined marker. Comments only; no test changed what it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tests were split by feature rather than by the file they exercise, so posterior_sampling.py had no test file of its own and Analysis1d had two. The sampler's tests now live in test_posterior_sampling.py under one TestPosteriorSampler, with the old class names as section banners, and the four tests that are really about Analysis1d's cached fitter move into TestAnalysis1d. No test changed what it does; the same 31 + 4 tests run as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Analysis and ParameterAnalysis each had a second test file, and the sampler had none of its own. The sampler's tests, whichever analysis drives them, now live in test_posterior_sampling.py under TestPosteriorSampler and TestMultiQPosteriorSampler; the fitter, chain parameter and label tests move into TestAnalysis and TestParameterAnalysis. Old class names became section banners. The multi-Q and ParameterAnalysis helpers keep distinct names in the merged file, since their signatures differ from the single-Q ones. The same 1660 tests run as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- extend() now verifies the chain holds the same parameters, not just the same number, and refuses to resume after a failed run or after the model or data changed - Parameter objects passed to sample(parameters=...) are validated against the free set the same way strings are - sampling with no free parameters and degenerate (min >= max) bounds raise clear errors before reaching BUMPS - parameters_at_bounds keys by unique_name so same-named per-Q parameters no longer collide, and guards empty draws - suggest_bounds flags non-finite fitted uncertainties for attention - save() refuses to write an empty label sidecar; loading one warns like a missing sidecar - colliding display labels get positional suffixes in the sidecar so save/load resolves each column to its own parameter - plot_posterior_predictive omits error bars when the data carries no variances (new Experiment.has_variances) - posterior plots validate draws/logp up front, name NaN columns, and share x-limits per corner column - document that sampling runs are not seedable Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # src/easydynamics/analysis/posterior_sampling.py # tests/unit/easydynamics/analysis/test_posterior_sampling.py # tests/unit/easydynamics/utils/test_posterior_plotting.py
- sampling one Q index independently now clears a stale simultaneous chain, so summary(), set_parameters_to_median() and plot_corner() report the run the user just made instead of the old one - extend() and save() after an independent run explain that the chains live on the per-Q analyses instead of resuming or saving the stale simultaneous chain; a genuinely failed run keeps its own message - Q_index arguments are validated like every Analysis method, so a negative index raises instead of silently wrapping - the gathered summary resolves each per-Q chain through its own saved labels, so chains loaded from disk keep names and units - warnings are attributed to the caller on both the single-Q and multi-Q paths, and the corner-plot slider forwards plot kwargs - the multi-Q integration tests share one independent sampling run, assert the straight line is actually recovered, and the extend test no longer mutates the shared fixture Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- plot_marginal(parameter) renders one parameter's posterior histogram with the median and the 16/84 percentile interval summary() reports, resolving labels the same way sample(parameters=...) does - plot_correlations() renders the Pearson correlation matrix of the chain with annotated cells, a diverging colormap and masked cells for constant columns - sample(progress=True) and extend(progress=True) report sampling progress through the Sampler's progress_callback, closing the line with an explicit done marker because BUMPS' own step estimate assumes the wrong chain count - the 95 percent predictive band needed no change: credible_interval already exists on plot_posterior_predictive Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After independent per-Q sampling the multi-Q sampler now presents a Q slider instead of refusing: - plot_posterior_predictive builds the per-Q data, median and credible band into a scipp DataGroup and renders it through plopp exactly like plot_data_and_model; plopp cannot shade a band on sliced lines, so the slider view draws labelled band edges while the Q_index path keeps the shaded band - plot_trace, plot_marginal and plot_correlations take Q_index for a single figure, show a slider in a notebook, and otherwise name the sampled Q indices - the matplotlib sliders render every figure once up front and only swap PNG bytes on a move, so dragging tracks smoothly with continuous updates instead of re-rendering per change - per-Q energy grids are NaN-padded onto the common grid through the finite mask, so masked points draw as gaps Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The tutorial now demonstrates plot_marginal and plot_correlations from the sampled chain, progress=True on the sampling call, the 95 percent predictive band option, the Q slider that every posterior plot offers over independent chains, and notes that runs are not seedable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The progress reporter closes through try/finally instead of a bare re-raise, and the plotting validation errors are documented in the form the docstring linter expects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Q-slider cells go through the plopp slicer, which refuses the inline backend; every plopp-using tutorial already runs %matplotlib widget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Label error. Requires at least 1 of: [bot] release, [scope] bug, [scope] documentation, [scope] enhancement, [scope] maintenance, [scope] significant. Found: |
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
1 participant
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.
Re-targeted continuation of #239, which was squash-merged into its stale stack base (
bayesian-analysis) instead of develop after #238 landed. Same content: the single-namespace import style, its tests and documentation.🤖 Generated with Claude Code