Interim updates by rozyczko · Pull Request #232 · easyscience/core

rozyczko · 2026-04-13T13:11:55Z

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_callback parameter and a way to handle cancellation gracefully.

Added an optional progress_callback parameter to the fit methods in Fitter, MinimizerBase, MultiFitter and all minimizer subclasses, allowing users to receive iterative progress updates and cancel fits.
Implemented progress callback integration in LMFit: added _create_iter_callback to wrap the user callback, constructed detailed progress payloads, and enabled fit cancellation by raising a new FitCancelled exception. Ensured parameter values are restored on cancellation or error.
Added the FitCancelled exception class to signal user-requested cancellation, and ensured proper restoration of parameter values on cancellation or error.

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['[scope] bug', '[scope] enhancement', '[scope] documentation', '[scope] significant', '[scope] maintenance']

rozyczko · 2026-04-13T13:12:26Z

Required for easyscience/EasyReflectometryApp#290

damskii9992

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.

damskii9992 · 2026-04-20T12:28:43Z

        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)


shouldn't this be fit_results.nfev?

No, this is correct for DFO-LS.
nf = number of objective evaluations
DFO doesn't follow SciPy style OptimizeResults API

damskii9992 · 2026-04-20T12:37:21Z

+            chi2_val = self.chi2
+            reduced_val = self.reduced_chi2
+            if not np.isfinite(chi2_val) or not np.isfinite(reduced_val):
+                raise ValueError


This error should probably have some context

This ValueError is not a user-facing error. It is raised only to be caught immediately by the except Exception in line 54, after which chi2 and reduced_chi2 are rendered as N/A

Even then, in the future we might change some code elsewhere to the catch exception and simply re-raise the error here. Without the error context, such re-raises would be hard to diagnose. It never hurts to have an descriptive error-message, even if it isn't currently being used.

I don't think this is necessary, but I will begrudingly accept the request.

updates

preparation for `interim_updates` branch merge

damskii9992

I would still like to see a tutorial or something which showcases how the progress callback might actually be used, not just a MagicMock unittest.

damskii9992 · 2026-04-25T18:26:45Z

        results.y_obs = self._cached_model.y
        results.y_calc = self.evaluate(results.x, minimizer_parameters=results.p)
        results.y_err = self._cached_model.dy
        results.n_evaluations = n_evaluations


I probably wouldn't save n_evaluations here, but rather iterations, since what we want to report isn't how many times the fit has evaluated the model function, but rather how many iterations the fit has run.

To be somewhat pedantic: both number of iterations and number of evaluations can be relevant parameters.

Cross-backend consistency (what I said earlier): LMFit.nfev and DFO-LS.nf both count objective-function calls, so n_evaluations matches.
I would like to be consistent and report just this.
I will document explicitly in the BUMPS docstring that for BUMPS this is objective evaluations, not iterations.

Then I would like to require that we ALSO report iterations in all of the minimizers. Because I only see a use-case for iterations, not for objective evaluations.

Users may be interested in the number of objective evaluations if the objective function is very time-consuming to calculate. A completely hypothetical example could be if the function involves a full or partial reduction workflow for an ESS instrument ;)

As hinted at above, I'm not against reporting both

I don't really see the need to confuse users but OK. We can add this attribute.
Done

damskii9992 · 2026-04-25T18:51:28Z

+    def __call__(self, history):
+        self.last_step = int(history.step[0])
+
+


We are only supposed to have 1 class per python file. I could accept 1 additional small class, but now you're reaching 2-3 small additional classes, so I think they belong in separate files in separate sub-folders.

Those are very small, single task classes which apply directly to Bumps class functionality. Splitting them and placing in say, utils.py would mean we could reuse them elsewhere. This is not the case.
If, at some point, we find these can be instantiated by another class, I'd pull them out.

I still think I'd prefer them pulled out into separate files in a seperate folder, say BUMPS_utils, and then a file for each class. I like clean file structures.

then we should probably spell it out in an ADR

damskii9992 · 2026-04-25T19:50:24Z

+        method_str = method_dict.get('method', self._method)
+        fitclass = self._resolve_fitclass(method_str)
+
+        step_counter = _StepCounterMonitor()


Does this change with our latest changes #231? We don't need the minimal step counter for the FitResults now, right? Since we get the n_evaluations there.

damskii9992 · 2026-04-25T20:51:57Z

+        progress_callback: Optional[Callable[[dict], Optional[bool]]] = None,
+        callback: Optional[Callable[[DFOCallbackState], None]] = None,
+        callback_every: int = 1,
+        callback_on_improvement_only: bool = False,


You did not separate this out into a different PR, so I guess we should discuss it here. Shouldn't these options also be available in the other minimizers? :)

damskii9992 · 2026-04-25T21:01:21Z

+            1 for parameter in params.values() if getattr(parameter, 'vary', False)
+        )
+        degrees_of_freedom = residual_array.size - varied_parameter_count
+        reduced_chi2 = chi2 / degrees_of_freedom if degrees_of_freedom > 0 else chi2


Same question as for DFO, does LMFIT not calculate chi2 and can't you just use that instead of calculating it again every iteration?

Nope. lmfit's iter_cb signature is (params, iter, resid, *args, **kwargs). There is no pre-computed chi2 passed in. LMFIT's MinimizerResult.chisqr is only available after the fit completes, not during iteration. So summing residuals**2 inside the callback is the correct approach.

rozyczko · 2026-04-26T15:38:22Z

I would still like to see a tutorial or something which showcases how the progress callback might actually be used, not just a MagicMock unittest.

Added progress-callback.ipynb

damskii9992 · 2026-04-27T20:25:55Z

+    "        f'final reduced chi2 = {res.reduced_chi2:.4f}, '\n",
+    "        f'n_evaluations = {res.n_evaluations}'\n",
+    "    )"
+   ]


I ran this notebook, and DFO returned:
final reduced chi2 = 0.0000
Which is obviously wrong . . .

In dfo, _gen_fit_results was assigning results.y_err = weights. But weights are the residual weights (1/σ - they get multiplied into residuals by _make_model, and FitResults.chi2 computes ((y_obs - y_calc) / y_err)**2, expecting y_err to be σ. LMFit and Bumps both correctly invert weights to σ; DFO did not. With weights ≫ 1, chi² collapsed toward 0; with weights = 1, the bug was masked.

damskii9992

Otherwise, pending answers/resolutions to my last 2 comments. I can finally approve this PR. You should also update the ADR to reflect the latest decisions :)

rozyczko added 2 commits April 13, 2026 13:59

initial commit

9d29d8f

ruff

2f66a46

github-actions Bot requested changes Apr 13, 2026

View reviewed changes

rozyczko added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority [area] base classes Changes to or creation of new base classes labels Apr 13, 2026

github-actions Bot approved these changes Apr 13, 2026

View reviewed changes

rozyczko added 5 commits April 14, 2026 13:06

updated BUMPS

2766be8

added DFO changes, streamlined implementation

92036d4

pixi run docs-format-fix

c60bf5a

changes after internal CR

53a80eb

assure fit cancellation enables undo

7858954

rozyczko marked this pull request as ready for review April 17, 2026 08:01

damskii9992 reviewed Apr 20, 2026

View reviewed changes

rozyczko added 9 commits April 21, 2026 08:45

removed fit cancellation from the PR. This one is only about the interim

665e99e

updates

PR review fixes

be61cb9

ruff changes

372de24

added unnecessary exception descriptor

4fc6d4b

Merge/develop into interim (#235)

78d4ddc

preparation for `interim_updates` branch merge

Interim conflicts (#236)

7e173c7

Merge branch 'develop' into interim_updates

57a65ff

fixes after merge

8a8fb84

minor fixes to functionality

68288c3

rozyczko requested a review from damskii9992 April 25, 2026 07:50

damskii9992 reviewed Apr 25, 2026

View reviewed changes

rozyczko added 2 commits April 26, 2026 20:46

PR review comments addressed

ec4ca68

ruff format on the notebook

26679b1

rozyczko added 2 commits April 27, 2026 17:40

let's show iterations as well. why not

d5f1aa1

refactored out util classes

da4c62d

damskii9992 reviewed Apr 27, 2026

View reviewed changes

damskii9992 approved these changes Apr 27, 2026

View reviewed changes

fixed weighing for DFO

7107665

rozyczko mentioned this pull request Apr 28, 2026

DFO specific callbacks need extension to other minimizers #237

Open

removed dfo specific callbacks

2c1321b

rozyczko merged commit 4be7c47 into develop Apr 28, 2026
33 checks passed

		def __call__(self, history):
		self.last_step = int(history.step[0])

Conversation

rozyczko commented Apr 13, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

rozyczko commented Apr 13, 2026

Uh oh!

damskii9992 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damskii9992 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rozyczko commented Apr 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damskii9992 left a comment

Choose a reason for hiding this comment

Uh oh!