Conversation
rozyczko
added
[area] fitting
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
I am ALMOST done reviewing this behemoth of a PR. Still need to look at sampler_dream.py and the tests. but the day is almost over and I figured I should send off these comments now.
|
You will need to change the tutorial in this PR too . . . |
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
Please also remove all the "Optional" typehints in the code and replace them with | None as is the new convention.
|
|
||
| def __init__( | ||
| self, | ||
| fitter: 'Fitter', |
There was a problem hiding this comment.
Wasn't a big part of this re-factoring done to also ensure that you didn't need a Fitter to create a Sampler?
There was a problem hiding this comment.
The refactor decoupled sampling from the minimizer, not from the Fitter. Fitter still has reshaping and fit-function wrapping.
One thing: __init__ requires hasattr(fitter, 'minimizer') but the minimizer is never used. I'll fix that.
| def validate_run_settings(samples: int, burn: int, thin: int) -> None: | ||
| """Validate the DREAM run settings. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| samples : int | ||
| Number of raw samples to draw; must be a positive integer. | ||
| burn : int | ||
| Burn-in generations to discard; must be a non-negative integer. | ||
| thin : int | ||
| Thinning interval; must be a positive integer. | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If any value is out of range or not an integer. | ||
| """ | ||
| # bool is a subclass of int, so ``samples=True`` would otherwise pass as | ||
| # ``samples=1``; these checks are strict (``10.0`` is rejected), so | ||
| # booleans must be rejected too. | ||
| if not isinstance(samples, int) or isinstance(samples, bool) or samples <= 0: | ||
| raise ValueError('samples must be a positive integer.') | ||
| if not isinstance(burn, int) or isinstance(burn, bool) or burn < 0: | ||
| raise ValueError('burn must be a non-negative integer.') | ||
| if not isinstance(thin, int) or isinstance(thin, bool) or thin < 1: | ||
| raise ValueError('thin must be a positive integer.') |
There was a problem hiding this comment.
Why factor this into a separate module? It's only used by the DreamSampler, so shouldn't it live there?
There was a problem hiding this comment.
Also, it is only used in 1 place, so why even factor it out at all? It's more readable to just have the code where it is used, if it is only used once . . .
There was a problem hiding this comment.
moved to samples/validation.py, dropped from bumps_utils/init.py, exported from samplers/init.py
There was a problem hiding this comment.
But why not move it into the DreamSampler class? It's only used in that class, right?
There was a problem hiding this comment.
Potentially usable by other Samplers, barred the docstring change from DREAM run settings
| validate_run_settings(samples, burn, thin) | ||
| validate_arrays(x, y, weights) |
There was a problem hiding this comment.
Should our validations belong in the Sampler class or here? Right now you validate in both places . . .
I think validations should belong in Sampler as long as they aren't engine-specific. Which means the engines can assume that whatever is passed to them is valid.
There was a problem hiding this comment.
It is not actually duplicated. Sampler checks numeric/non-scalar/non-empty; validate_arrays checks shape match, finiteness, positive weights.
They are more complementary than dupes. And validate_arrays cannot move into Sampler regardless, since all three minimizers call it from Fitter.fit...
No changes needed. I will need to change the tutorials for the |
I think we have misaligned our idea for the public API a little bit. I wanted For convenience sake, I thought it'd be prudent to have a classmethod to construct a |
Usually you want to do the fitting before running the sampling, so setting the fitter on the Sampler sort of makes sense. |
2 participants
Core refactoring and shared logic:
EngineBase(engine_base.py) as a new base class for all evaluation engines (minimizers and samplers), with centralized parameter handling, fit function wrapping, and evaluation logic.MinimizerBaseso it inherits fromEngineBase. Cleaned up the constructor and removed duplicated parameter caching logic.Bumps integration and utilities:
bumps_utils/problem.py, includingbuild_curve_problem,to_bumps_parameter,parameter_names, andparameter_snapshot. These are now imported and exposed inbumps_utils/__init__.py.bumps_utils/validation.py) with functions for validating run settings and input arraysAPI changes and deprecations:
mcmc_sample()now lives onSamplerclass directly. The method now routes calls toSampler(...).sample(...)progress_callbacksignature inMinimizerBase.fitto clarify that its return value is ignored and to align with the new shared engine base.