Fixed minimizer behaviour on exceeded iterations by rozyczko · Pull Request #231 · easyscience/core

rozyczko · 2026-04-09T11:57:51Z

This adds improvements to the fitting result reporting and evaluation tracking across all minimizer engines, ensuring that the number of function evaluations and relevant messages are consistently captured and propagated.

All minimizer engines (Bumps, DFO, LMFit) now populate n_evaluations (number of function evaluations) and message (status or error message) fields in FitResults, and these fields are propagated in multi-dataset fitting.
The Bumps minimizer uses a new _EvalCounter wrapper to count function evaluations and detect when the maximum evaluation budget is reached, updating the success flag and message accordingly.
The DFO minimizer now raises FitError only for real failures, and considers both success and max function evaluation warnings as successful fits, improving error reporting and testability.

henrikjacobsenfys

Looks good to me apart from a tiny nitpick.

This may also be a good opportunity to make a __repr__ method for the FitResults class?

rozyczko · 2026-04-13T15:02:52Z

This may also be a good opportunity to make a __repr__ method for the FitResults class?

Good idea!
Added these

   lines = [
        f"FitResults(success={self.success}",
        f"  n_pars={self.n_pars}, n_points={len(self.x)}",
        f"  chi2={self.chi2:.4g}, reduced_chi={self.reduced_chi:.4g}",
        f"  n_evaluations={self.n_evaluations}",
        f"  minimizer={self.minimizer_engine.__name__ if self.minimizer_engine else None}",
    ]

damskii9992

Generally looks good. I have some questions/potential changes before approval.
Also, do we want to raise an error or a warning when a fit does not converge withing max_iterations?

damskii9992 · 2026-04-21T08:36:51Z

            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


Can you explain this? What is fit_results.EXIT_SUCCESS?

And should we raise a warning if the fit did not succeed?

In DFO-LS, EXIT_SUCCESS is 0 when “objective is sufficiently small” or “rho has reached rhoend”. Positive codes are warning exits, and negative codes are hard errors.

whether to raise a warning: not for every non-success. Hard failures already become an exception in minimizer_dfo, so warning on those would be redundant. But for warning-style exits that still return results, especially EXIT_MAXFUN_WARNING, adding a UserWarning would be reasonable if we want consistency with BUMPS, which warns when it hits the evaluation budget.

Added warning and unit test

Okay, but what about the fit_results.flag? I assume this can be other values than 0. Do we risk this returning True when the flag and EXIT_SUCCESS are identical values different from 0? What is EXIT_SUCCESS value when we hit a EXIT_MAXFUN_WARNING?

No, there is no risk from the value being nonzero by itself.
This is a straight equality check:

results.success = fit_results.flag == fit_results.EXIT_SUCCESS

It only returns True when the flag exactly matches success const. It does not depend on success being 0 specifically. Even if a library used 137 for success, this comparison would still be correct as long as fit_results.flag and fit_results.EXIT_SUCCESS were both 137 for a real success.

For DFO we have:

EXIT_SUCCESS = 0 EXIT_MAXFUN_WARNING = 1 EXIT_SLOW_WARNING = 2 EXIT_FALSE_SUCCESS_WARNING = 3

hard errors have negative values

The line does what we want:
normal success -> True
maxfun warning -> False
any other non-success flag -> False

I added a comment above the assignment to spell it out.

But what about fit_results.flag, what values can that take? Only 0 and 1?

But what about fit_results.flag, what values can that take? Only 0 and 1?

Line 271: DFO results object is returned, if results.flag is either 0 (success) or 1 (MAXFUN_WARNING), the results object is returned. Otherwise it is assumed the fit failed and assert raised.

So in line 219, where this is actually being checked, flag_results.flag is only 0 or 1.

Ahh, now I think I get it. DFO sets fit_results.flag to some integer depending on if the fit failed (negative), succeeded cleanly (0) or succeeded with a warning (positive). To remember what each of these integers means, they're also assigned to the attributes EXIT_SUCCESS, EXIT_MAXFUN_WARNING etc.
In other words, the flag attribute is a variable and the EXIT attributes are static constants.
So when you compare, you're just checking the flag against the static attribute.
I guess this is done so that they can change the flag integers for different fit terminations without breaking code, or for more easily readable syntax . . . Hmm, interesting.
Could you boil my realization down and add as a comment? Either instead of your comment or as an addendum :)

damskii9992 · 2026-04-21T08:39:27Z


-        if 'Success' not in results.msg:
-            raise FitError(f'Fit failed with message: {results.msg}')
+        if results.flag in {results.EXIT_SUCCESS, results.EXIT_MAXFUN_WARNING}:


Could you also explain this? Maybe add a comment?

Added comment

damskii9992 · 2026-04-21T08:40:11Z

+        except FitError:
+            for key in self._cached_pars.keys():
+                self._cached_pars[key].value = self._cached_pars_vals[key][0]
+            raise


Codecov says this misses a test :)

test added. you know we shouldn't really aim for 100% coverage? :)

damskii9992 · 2026-04-21T08:46:02Z

        results.y_calc = fit_results.best_fit
        results.y_err = 1 / fit_results.weights
+        results.n_evaluations = fit_results.nfev
+        results.message = fit_results.message


We should probably either raise an error or a warning when max_evaluations have been reached.

As mentioned earlier, I would prefer to let the caller decide on what to do with this information. Let's spit a warning and be gracious wrt. max_evaluations.

damskii9992 · 2026-04-21T08:49:19Z

+
+    assert result.success is False
+    assert result.n_evaluations is not None
+    assert result.n_evaluations > 0


Since the fit reached max evaluations, shouldn't this be:

assert result.n_evaluations == 3

n_evaluations is the actual backend-reported evaluation count, not necessarily the configured budget. In this case, n_evaluations is 10.

But doesn't that mean that setting max_evaluations doesn't currently work? If the max is 3, the number of iterations should be 3 unless it converged in under 3 iterations (doubtful). I guess this relates to my earlier comment that BUMPS runs multiple function evaluations before actually starting the fit.

yes, BUMPS is interesting.

Our n_evaluations comes from _EvalCounter.call, which counts every objective invocation.
Simplex nethod evaluates multiple points per optimizer iteration + does work before the main loop starts by building the initial simplex.

So with steps = 3 getting 10 n_evaluations is plausible. The solver is not running 10 iterations but rather using up to 3 simplex steps, and those steps require multiple objective calls.

f.max_evaluations = 3 for BUMPS currently means: maximum optimizer steps
result.n_evaluations means: actual objective-function calls observed.
That is why I changed the BUMPS message to say “maximum optimizer steps” rather than “maximum evaluations”.

The real issue is naming: in BUMPS, max_evaluations is currently mapped to steps, so the public API name is more general than the actual behavior.

And we can't set the actual mac number of optimizer steps explicitly...

damskii9992 · 2026-04-21T08:51:30Z

        assert domain_fit_results.y_calc == 'evaluate'
        assert domain_fit_results.y_err == 'dy'
+        assert domain_fit_results.n_evaluations == 7
+        assert domain_fit_results.message == 'Fit stopped: reached maximum evaluations (3)'


Huh? max evaluations was 3 but it got 7 iterations? What?

minimizer._eval_counter = MagicMock(count=7) in line 205.

rozyczko · 2026-04-21T12:51:54Z

Generally looks good. I have some questions/potential changes before approval. Also, do we want to raise an error or a warning when a fit does not converge withing max_iterations?

I would prefer to just emit the warning and let the caller check the results object for convergence.
Or maybe we should all have a discussion about that?

damskii9992 · 2026-04-22T08:40:20Z

+
+    def __call__(self, *args, **kwargs):
+        self.count += 1
+        return self._fn(*args, **kwargs)


Thinking about this now, but you're tracking fit iterations by tracking how many times the model function is evaluated. This is not a correct representation of the number of fit iterations, as some fitting routines evaluate the model function multiple times per fitting step in order to best choose the next fitting step.
Optimizers like BUMPS, for example, run multiple model evaluations at the start of each fit.

This might be the best we CAN do, if we can't get the actual fit iterations from the back engine, but it's worth mentioning.

You might want to add to the integration tests where we actually run the fits and check the iteration counters.

This is the best we can currently do.

The actual iteration counters might differ from machine to machine, from OS to OS, I don't think it's going to be easy to catch the right situation, but I'll give it a try

I just checked, the number of iterations used by Bumps is saved in the engine_result.nit:

I guess we may use nit, assuming BUMPS is consistent with it.

rozyczko · 2026-04-23T09:00:12Z

I assume that this is now ready for merge

initial implementation

db4f4b0

rozyczko added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority labels Apr 9, 2026

github-actions Bot approved these changes Apr 9, 2026

View reviewed changes

rozyczko added 5 commits April 9, 2026 14:02

ruff

2e63985

added missing collection base files

f88120d

removed unneeded files

3dbc23a

added a test

4effb57

ruff

41c2224

henrikjacobsenfys approved these changes Apr 10, 2026

View reviewed changes

Comment thread tests/integration/fitting/test_fitter.py

rozyczko added 2 commits April 13, 2026 16:52

addressed code review points

d5321a2

ruff format

f83ef52

AndrewSazonov reviewed Apr 13, 2026

View reviewed changes

Comment thread src/easyscience/fitting/minimizers/utils.py Outdated

reduced_chi -> reduced_chi2

ce6feeb

damskii9992 reviewed Apr 21, 2026

View reviewed changes

Code review comments addressed

67d96ad

damskii9992 reviewed Apr 22, 2026

View reviewed changes

rozyczko added 2 commits April 22, 2026 11:13

added comment, added integ test

9267e57

use .nit for BUMPS it count

d85a723

rozyczko merged commit 8e6d351 into develop Apr 23, 2026
33 checks passed

Conversation

rozyczko commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrikjacobsenfys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rozyczko commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

damskii9992 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damskii9992 Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

rozyczko commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rozyczko commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

rozyczko commented Apr 9, 2026 •

edited

Loading

rozyczko commented Apr 13, 2026 •

edited

Loading

damskii9992 Apr 22, 2026 •

edited

Loading

rozyczko commented Apr 21, 2026 •

edited

Loading