BUG: Optimize: NewtonCG min crashes with xtol=0

[TimButters]

The minimize function crashes if xtol is set to 0. This is because the initial value of a loop variable is 2 * xtol, which is intended to ensure the loop runs at least once. However, if xtol = 0 then the loop never runs, a variable isn't instantiated that is used outside of the loop, and an UnboundLocalError is raised.

To fix this, the initial value of the loop variable is set to the max float value.

Reference issue

Closes #20214

What does this implement/fix?

UnboundLocalError is thrown if xtol is set to 0 for the newton-cg method in minimize.

Additional information

[lucascolley]

LGTM!

[tylerjereddy]

I'll let the optimize regulars chime in on whether a regression test is sensible to add, but this slightly modified version of the original reproducer fails before/passes after and seems fast:

--- a/scipy/optimize/tests/test_optimize.py
+++ b/scipy/optimize/tests/test_optimize.py
@@ -3120,3 +3120,19 @@ def test_sparse_hessian(method, sparse_type):
     assert res_dense.nfev == res_sparse.nfev
     assert res_dense.njev == res_sparse.njev
     assert res_dense.nhev == res_sparse.nhev
+
+
+def test_gh_20214():
+    def cosine(x):
+        f = np.cos(x[0])
+        return f
+
+    def jac(x):
+        ret = -np.sin(x)
+        return ret
+
+    result = optimize.minimize(cosine,
+                               x0=[0.1],
+                               jac=jac,
+                               method="newton-cg",
+                               options=dict(xtol=0, maxiter=2))

[h-vetinari]

At least one error from #20214 occurs elsewhere, namely update is undefined in this branch:

scipy/scipy/optimize/_optimize.py

Lines 2118 to 2120 in cd60e81

	else:
	if np.isnan(old_fval) or np.isnan(update).any():
	return terminate(3, _status_message['nan'])

Looking at c8c78d7, this commit failed to rename that instance of update to update_l1norm. CC @lorentzenchr

We need a test that exercises this else-branch in any case. I haven't checked if @tylerjereddy's case does that, but to reach the else: clause of the while: loop, we must achieve update_l1norm <= xtol without having triggered any of the early return statements in the while body.

[j-bowhay]

Thanks @TimButters lgtm but I will wait to see if others are happy before merging

[h-vetinari]

I'm still unsure about the right initial value for update_l1norm, though I can see the argument that it's being updated at the end of the loop. Still, the interplay with the shape of x is a mystery to me. The code seems very brittle w.r.t. to putting in x0=[1, 2] or something like that.

Unless we can figure this out quickly and comprehensively, I'd prefer an immediate fix for the unboundname error to go into 1.13, and the rest to be fixed for 1.14.

[lorentzenchr]

Is there a test for the „nan-path“?

[TimButters]

Is there a test for the „nan-path“?

@lorentzenchr yes, there is an existing test test_nan_values that checks this branch.
Line 1494 in test_optimize.py.

Not going to stand in the way here as I don't know the code well enough and I believe this is in good hands; as long as the UnboundNameError is fixed I'm happy. :)

[TimButters]

@j-bowhay @lucascolley I think we have consensus now, would one of you be happy to merge this?

[lucascolley]

thanks @TimButters , I'll give it a few days for any more comments before merging.

As a note for the future, I'd recommend that you leave resolving conversations to maintainers - a thorough reviewer will probably want to check any conversations that were resolved by the author, so it is easier to leave them as is.

[TimButters]

Thanks for the feedback @lucascolley, I thought I was doing the right thing there. I’ll leave them open in the future.

[lucascolley]

I'll give it a few days for any more comments before merging.

Here we are, thanks @TimButters !

[j-bowhay]

Thanks @TimButters a great couple of first contributions, keep them coming!