Make Sampler completely Fitter-agnostic - #310
Conversation
rozyczko
added
[scope] maintenance
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## sampler-engine-structure-280 #310 +/- ##
================================================================
+ Coverage 83.64% 83.82% +0.17%
================================================================
Files 68 69 +1
Lines 5253 5272 +19
================================================================
+ Hits 4394 4419 +25
+ Misses 859 853 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
One potentially important comment and some minor comments. Otherwise it looks good to me :)
| "where $\\theta$ are the model parameters, $d$ is the observed data, $p(d \\mid \\theta)$ is the likelihood, and $p(\\theta)$ is the prior. In `easyscience`, the `min`/`max` bounds of a `Parameter` are interpreted as a **uniform prior**, and a Gaussian likelihood is constructed from the data and supplied weights.\n", | ||
| "\n", | ||
| "`easyscience` exposes a Bayesian Markov-chain Monte Carlo (MCMC) sampler through the `Sampler` class. Under the hood this uses BUMPS' DREAM sampler, so the underlying minimizer must be switched to BUMPS.\n", | ||
| "`easyscience` exposes a Bayesian Markov-chain Monte Carlo (MCMC) sampler through the `Sampler` class. It is a parallel entry point to `Fitter`: both take a model object and a model function, so you can sample without ever creating a `Fitter`. Under the hood `Sampler` uses BUMPS' DREAM sampler, so the `bumps` package must be installed.\n", |
There was a problem hiding this comment.
It is a parallel entry point to
Fitter: both take a model object and a model function, so you can sample without ever creating aFitter.
Would remove this comment.
| "does not need a `Fitter` at all, and you can sample straight from the initial parameter\n", | ||
| "values.\n", |
There was a problem hiding this comment.
Would also just remove this comment :)
| "We now draw samples from the posterior distribution $p(\\theta \\mid d)$ using the BUMPS DREAM (DiffeRential Evolution Adaptive Metropolis) algorithm. DREAM is an ensemble MCMC method that runs multiple chains in parallel and automatically tunes the proposal distribution.\n", | ||
| "\n", | ||
| "DREAM only works with the BUMPS minimizer. We reuse the ``mle_fitter`` created above — any configured `Fitter` would do, and it does not have to have been fitted — switch it to BUMPS, and create a `Sampler` instance bound to the fitter and data. Calling `sampler.sample()` returns a `SamplingResults` object with the following attributes:\n", | ||
| "Create a `Sampler` from the same `parameter_container` and `intensity_model` we gave the `Fitter`, bound to the data. No `Fitter` is involved: the sampler only needs the model object, the model function, the data and the `bumps` package. Calling `sampler.sample()` returns a `SamplingResults` object with the following attributes:\n", |
There was a problem hiding this comment.
No
Fitteris involved: the sampler only needs the model object, the model function, the data and thebumpspackage.
Would remove this line :)
| wrapped_fns = [ | ||
| inject_x(this_fun, this_x, flatten=flatten) | ||
| for this_x, this_fun in zip(real_x, fit_functions) | ||
| ] |
There was a problem hiding this comment.
Nice, this is actually cleaner when factored out. No more setting the ._fit_function in the MultiFitter for each function :)
| self._fit_object = fit_object | ||
| self._update_minimizer(self._enum_current_minimizer) | ||
|
|
||
| def _fit_function_wrapper( |
There was a problem hiding this comment.
Can't we just forego having this method now? It is just an alias . . .
| # Make a 'dummy' x array for the fit function | ||
| x_for_fit = np.array(range(y_new.size)) | ||
| return x_for_fit, x_new, y_new, weights, x_shape | ||
| _precompute_reshaping = staticmethod(reshape_dataset) |
There was a problem hiding this comment.
And same with this one, can't we just get rid of it now? :)
And just call reshape_dataset where it is needed.
| w_new = np.hstack(w_new) | ||
| x_fit = np.linspace(0, y_new.size - 1, y_new.size) | ||
| return x_fit, x_new, y_new, w_new, dims | ||
| _precompute_reshaping = staticmethod(reshape_datasets) |
There was a problem hiding this comment.
And the same here, can't we just use the functions directly? Is there any reasons to have the aliases?
Except for avoiding to change the few lines where they're used?
MultiFitter overrides both. Multifitter binds reshape_datasets (plural) instead of reshape_dataset, and its wrapper calls
Removing these means that either No downstream package (reflectometry, dynamics) overrides them, so there is no external constraint either way. I would suggest keeping as-is and maybe add a comment on each saying it is the override point for MultiFitter? |
2 participants
Addressing @damskii9992's issue in #287 (comment)
Made
SamplerfullyFitter-independent.This is a separate PR on top of #287 to allow for easier review.