Repository navigation
BUG: lsq trf gives x=1e-10 if x0 is near a bound - #18896
Conversation
Closes scipy#18793. Previously, if x0 was within about 1e-10 of a bound and was a was close to the correct solution, least squares would gives x = [ 1e-10 ] if using trf. Now, it gives x = x0, which is the same behavior as dogbox. Previously, make_strictly_feasible was called before trf but after an initial bounds check. Now, make_strictly_feasible is not called prior to trf.
j-bowhay
added
the
defect
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
Seems straightforward enough, though not my area of expertise. One comment--any reason not to include the following regression test that fails on main and passes here in < 0.04s?
--- a/scipy/optimize/tests/test_least_squares.py
+++ b/scipy/optimize/tests/test_least_squares.py
@@ -809,3 +809,12 @@ def test_fp32_gh12991():
# used a step size for FP64 when the working space was FP32.
assert res.nfev > 2
assert_allclose(res.x, np.array([0.4082241, 0.15530563]), atol=5e-5)
+
+
+def test_gh_18793():
+
+ def chi2(x):
+ return (x-1e-12)**2
+
+ res = least_squares(chi2, x0=1.1e-12, bounds=(0, np.inf))
+ assert_allclose(res.x, 1.1e-12)
tylerjereddy
added
the
backport-candidate
This is a regression test for scipy#18793. This checks that, if an initial condition is already close to the answer, trf does not return a result that is less accurate than the initial condition.
|
That makes sense to add! I changed Thank you for your review! |
|
Thanks, I squash-merged to facilitate backporting. Folks seemed in agreement on the fix in the issue, the CI failure isn't related, and there's a new regression test, so seemed good to go. |
tylerjereddy
removed
the
backport-candidate
|
got messages after upgrading Scipy from 1.11.1 to 1.11.2: /mambaforge/lib/python3.10/site-packages/scipy/optimize/_lsq/trf.py:234: RuntimeWarning: divide by zero encountered in divide Not sure if it is related. |
|
@sweettyler Yes this is noted by me as well. This should not have been merged, but a reduced step on make_strictly_feasible with rstep=0, would have already fixed this instead. You only have warnings, but see my related issue for a case where an optimization problem fails instead. |
|
Try to propose the desired behavior in a PR, pinging the folks involved, and add a regression test for the behavior you want preserved, since we obviously don't have such a test. |
PR scipy#18896 removed make_strictly_feasible because it makes solutions worse if they are close to the boundary. However, the downstream algorithm that eventually solves the optimization problem has some trouble with starting points that are on the boundaries of a bounded optimization problem. This either results in division by zero warnings, see scipy#19102, or in failure of finding the correct solution at all, which is added as a regression test in this commit. This PR brings back make_strictly_feasible but with the smallest possible step from the boundary a floating point allows by setting rstep=0 on the function make_strictly_feasible, which uses np.nextafter to find such a point, hereby respecting the strictly feasible point condition and minimizing boundary effects. See strictly feasible points in interior-point methods for more context https://nmayorov.wordpress.com/2015/06/19/trust-region-reflective-algorithm/
PR scipy#18896 removed make_strictly_feasible because it makes solutions worse if they are close to the boundary. However, the downstream algorithm that eventually solves the optimization problem has some trouble with starting points that are on the boundaries of a bounded optimization problem. This either results in division by zero warnings, see scipy#19102, or in failure of finding the correct solution at all, which is added as a regression test in this commit. This PR brings back make_strictly_feasible but with the smallest possible step from the boundary a floating point allows by setting rstep=0 on the function make_strictly_feasible, which uses np.nextafter to find such a point, hereby respecting the strictly feasible point condition and minimizing boundary effects. See strictly feasible points in interior-point methods for more context https://nmayorov.wordpress.com/2015/06/19/trust-region-reflective-algorithm/
PR scipy#18896 removed make_strictly_feasible because it makes solutions worse if they are close to the boundary. However, the downstream algorithm that eventually solves the optimization problem has some trouble with starting points that are on the boundaries of a bounded optimization problem. This either results in division by zero warnings, see scipy#19102, or in failure of finding the correct solution at all, which is added as a regression test in this commit. This PR brings back make_strictly_feasible but with the smallest possible step from the boundary a floating point allows by setting rstep=0 on the function make_strictly_feasible, which uses np.nextafter to find such a point, hereby respecting the strictly feasible point condition and minimizing boundary effects. See strictly feasible points in interior-point methods for more context https://nmayorov.wordpress.com/2015/06/19/trust-region-reflective-algorithm/
Reference issue
Closes #18793.
What does this implement/fix?
Previously, if x0 was within about 1e-10 of a bound and was a was close to the correct solution, least squares would gives x = [ 1e-10 ] if using trf. Now, it gives x = x0, which is the same behavior as dogbox.
Additional information
Previously, make_strictly_feasible was called before trf but after an initial bounds check. Now, make_strictly_feasible is not called prior to trf. As far as I could tell, that call of make_strictly_feasible was not doing anything useful. I am not sure why it was there in the first place.