Conversation
…, superceeding bounds.
There was a problem hiding this comment.
Pull request overview
This PR replaces parameter bounds with support for arbitrary scipy.stats frozen distributions as priors across FittingBase-derived fitting classes, enabling non-uniform priors (per #143) and updating tests/docs accordingly.
Changes:
- Introduce
priorssupport inFittingBaseand propagate it through Arrhenius/VTF and Yeh-Hummer fitting. - Add/adjust unit tests to cover prior initialization and usage.
- Update Arrhenius tutorial documentation to demonstrate providing priors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
kinisi/fitting.py |
Replaces bounds-based configuration with prior distributions; adds MAP objective and validation. |
kinisi/arrhenius.py |
Switches Arrhenius/VTF wrappers to pass priors into FittingBase. |
kinisi/yeh_hummer.py |
Updates YehHummer to accept priors and passes them to FittingBase; adjusts default prior construction. |
kinisi/tests/test_fitting.py |
Adds new unit tests for FittingBase priors and sampling. |
kinisi/tests/test_arrhenius.py |
Updates Arrhenius/VTF tests to use priors instead of bounds. |
kinisi/tests/test_yeh_hummer.py |
Updates YehHummer tests to exercise custom priors and invalid priors. |
docs/source/arrhenius_t.ipynb |
Updates tutorial to demonstrate priors-based API and explain scipy.stats.uniform. |
Comments suppressed due to low confidence (3)
kinisi/tests/test_yeh_hummer.py:67
- Leftover debug
printstatements in the test will pollute test output and can cause CI noise/failures when output is asserted.
assert 1e-4 < yh.shear_viscosity.value < 1e-2
def test_yeh_hummer_mcmc(self):
kinisi/arrhenius.py:161
VogelFulcherTammann.__init__still uses a bounds-like type annotation forpriors, butFittingBaseexpects a sequence of frozenscipy.statsdistributions. The current annotation is inconsistent with runtime validation.
def __init__(
self,
diffusion,
priors: tuple[
tuple[VariableLike, VariableLike], tuple[VariableLike, VariableLike], tuple[VariableLike, VariableLike]
]
| None = None,
) -> 'VogelFulcherTammann':
kinisi/fitting.py:204
max_aposterioriis annotated to returntuple[float]but currently returnsNone. Returning the fitted parameters keeps the API consistent with the type hint and withmax_likelihood.
self.data_group[name] = result[i] * self.parameter_units[i]
def mcmc(self, n_samples: int = 1000, n_walkers: int = 32, n_burn: int = 500, n_thin=10) -> None:
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = minimize(self.nll, x0).x | ||
| for i, name in enumerate(self.parameter_names): | ||
| self.data_group[name] = result[i] * self.parameter_units[i] | ||
|
|
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot please re-review |
…t slope/viscosity labeling
Addressed the remaining unresolved threads in commit
|
| @@ -1,4 +1,4 @@ | |||
| __version__ = '2.0.5' | |||
| __version__ = '2.1.0' | |||
There was a problem hiding this comment.
unrelated to the PR, but could be interesting to you https://github.com/ofek/hatch-vcs ( very easy to add PythonFZ/zndraw-joblib#2 ) and auto-updates the version based on git tags + you get the commit hash if you are working with a dev version
There was a problem hiding this comment.
Happy to welcome a PR that implemented this!
PythonFZ
left a comment
There was a problem hiding this comment.
Looks great. The only thing worth mentioning is, are we fine with removing the units previously available in the bounds? Are there any scenarios where this could cause issues?
|
Yeah, the units thing is a bit annoying. I had an idea to write a wrapper for |
PythonFZ
left a comment
There was a problem hiding this comment.
The wrapper is probably out of scope and the addition is quite valuable to tweak sampling from e.g. log-space distributed samples. LGTM 👍
This PR implements the ability to pass arbitrary scipy.stats objects as priors. This means that non-uniform priors can be passed to different FittingBase objects.
However, to achieve this, I have removed the
boundskeyword argument, which is an API change; hence, this will be a part of a 2.1.0 release to account for this.Closes #143