Replies: 2 comments
-
|
Proposed initial implementation |
Beta Was this translation helpful? Give feedback.
0 replies
-
|
Data flow after the refactor: |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
Context
PR #257 adds Bayesian MCMC sampling (resume/extend of BUMPS DREAM chains). The sampling entry point is a single
Samplerclass that binds one(x, y, weights)dataset to a configuredFitterand drives BUMPS' DREAM sampler throughBumps.mcmc_sample.Backend selection today is inside
Sampler._run: it checks that theactive minimizer's
package == 'bumps'and raises otherwise. That check is documented (in theSamplerclass and_rundocstrings) as the seam where a second backend would plug in.The wider fitting subsystem already has an established shape for "one abstract capability, several interchangeable backends":
AvailableMinimizers(an
Enumof apackage/method/enum_iddataclass), gated by*_engine_availableimport probes, plus afrom_string_to_enumhelper.MinimizerBase(
ABCMeta, with apackageclass attribute and@abstractmethods likefit,convert_to_pars_obj,supported_methods).LMFit,Bumps,DFO, one file each underminimizers/.factory()dispatching onminimizer_enum.packageto construct the right backend.The review of PR #257 raised the question of whether the sampler should adopt this same structure.
We can:
if package == 'bumps'guard, and adding a backend is a well-worn path._runBUMPS check is already the documented seam, so nothing is lost by waiting.This ADR exists to record that decision and, if/when we build the abstraction, to fix its target shape so it lands consistently.
Decision proposal
Defer the factory/enum/base-class structure until a second sampler backend is actually implemented, but commit now to the target shape below so the eventual refactor is mechanical rather than a fresh design.
Concretely:
Now (this PR / near term): keep the single
Samplerclass and thepackage == 'bumps'guard in_runas the backend seam. Do not introduceAvailableSamplers, aSamplerBaseABC, or a samplerfactory()for one backend.Trigger: the first concrete proposal for a second MCMC backend (e.g. an
emceeaffine-invariant sampler, or a second BUMPS algorithm exposed as a distinct sampler). At that point, extract the abstraction from the two real cases in the same PR that adds the second backend.Target shape (when the trigger fires) — mirror the minimizer subsystem:
AvailableSamplersenum infitting/available_samplers.py, entries gated by*_engine_availableprobes, carrying at leastpackage(and a stableenum_idif we want parity with the minimizer registry), plus afrom_string_to_enumhelper.SamplerBaseABC infitting/samplers/sampler_base.pywith apackageclass attribute and@abstractmethods for the backend-specific surface (sample,extend,save,load_state, and the state/results accessors). The persistence sidecar contract (save_chain/load_chain,_SIDECAR_SCHEMA_VERSION) and theSamplingResultsdataclass stay backend-agnostic and shared.fitting/samplers/(e.g.sampler_dream.py), each declaring itspackage.factory(sampler_enum, fitter, ...)dispatching onsampler_enum.package, replacing the inlinepackage == 'bumps'guard in_run.The current
Samplerbecomes either theSamplerBasesubclass for the DREAM/BUMPS backend or a thin data-binding orchestrator that delegates to a factory-constructed backend — decided when the second backend clarifies which responsibilities are genuinely shared.Alternatives considered
Rejected as premature: maximises uniformity but pays indirection for a single backend and risks encoding a one-case guess as the interface. Reconsider immediately if a second backend is already on the near-term roadmap rather than hypothetical.
Beta Was this translation helpful? Give feedback.
All reactions