Conversation
|
Required for easyscience/EasyReflectometryApp#290 |
rozyczko
added
[scope] enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
This PR needs an ADR explaining the software architecture design implemented and the rationale for why this design was chosen etc.
We probably also need a tutorial showcasing how the callback might be used. Both for ourselves as a kind of documentation but also for potential users.
I have not bothered looking at the unit tests.
|
|
||
| def _restore_parameter_values(self) -> None: | ||
| for key in self._cached_pars.keys(): | ||
| self._cached_pars[key].value = self._cached_pars_vals[key][0] |
There was a problem hiding this comment.
You probably also want to restore the error/variance here.
There was a problem hiding this comment.
And this method should be reused in the fit method of all the minimizers when fitting fails.
| method: Optional[str] = None, | ||
| tolerance: Optional[float] = None, | ||
| max_evaluations: Optional[int] = None, | ||
| progress_callback: Optional[Callable[[dict], Optional[bool]]] = None, |
There was a problem hiding this comment.
We don't type hint optional parameters with Optional anymore, use | None.
| history.requires(step=1) | ||
|
|
||
| def __call__(self, history): | ||
| self.last_step = int(history.step[0]) |
There was a problem hiding this comment.
Why not just use the actual class implemented in Bumps? StepMonitor in fitters seems to do exactly what you want.
| problem=self._problem, | ||
| iteration=int(history.step[0]), | ||
| point=np.asarray(history.point[0]), | ||
| nllf=float(history.value[0]), |
There was a problem hiding this comment.
nllf? What is that supposed to mean?
| tolerance # tolerance for change in parameter value, could be an independent value | ||
| ) | ||
| minimizer_kwargs['ftol'] = tolerance | ||
| minimizer_kwargs['xtol'] = tolerance |
There was a problem hiding this comment.
You probably shouldn't remove the comments here. They're good to have to remember what the difference between xtol and ftol is without having to find it in the Bumps source code. ( I really hate that they don't have a documentation).
| results = self._gen_fit_results(model_results, weights) | ||
| except FitError: | ||
| self._restore_parameter_values() | ||
| raise |
There was a problem hiding this comment.
Is this an empty raise statement? What happens then? O.o
| raise ValueError('Weights must be strictly positive and non-zero.') | ||
|
|
||
| if callback_every < 1: | ||
| raise ValueError('callback_every must be a positive integer.') |
There was a problem hiding this comment.
what about floats like 1.3?
| results.y_obs = self._cached_model.y | ||
| results.y_calc = self.evaluate(results.x, minimizer_parameters=results.p) | ||
| results.y_err = weights | ||
| results.n_evaluations = int(fit_results.nf) |
There was a problem hiding this comment.
shouldn't this be fit_results.nfev?
| if getattr(results, name, False): | ||
| setattr(results, name, value) | ||
| results.success = not bool(fit_results.flag) | ||
| results.success = fit_results.flag == fit_results.EXIT_SUCCESS |
There was a problem hiding this comment.
Consider raising a warning if the fit did not succeed.
| chi2_val = self.chi2 | ||
| reduced_val = self.reduced_chi2 | ||
| if not np.isfinite(chi2_val) or not np.isfinite(reduced_val): | ||
| raise ValueError |
There was a problem hiding this comment.
This error should probably have some context
2 participants
This pull request adds support for progress callbacks during fitting operations. The main goal is to allow users to receive progress updates and optionally cancel fits in progress. Added
progress_callbackparameter and a way to handle cancellation gracefully.Added an optional
progress_callbackparameter to the fit methods inFitter,MinimizerBase,MultiFitterand all minimizer subclasses, allowing users to receive iterative progress updates and cancel fits.Implemented progress callback integration in
LMFit: added_create_iter_callbackto wrap the user callback, constructed detailed progress payloads, and enabled fit cancellation by raising a newFitCancelledexception. Ensured parameter values are restored on cancellation or error.Added the
FitCancelledexception class to signal user-requested cancellation, and ensured proper restoration of parameter values on cancellation or error.