BUG: ENH: Check if guess for newton is already zero before checking derivative #8907

mikofski · 2018-06-05T18:06:46Z

fixes BUG: if zero derivative at root, then Newton fails with RuntimeWarning #8904
adds a test to see if RuntimeWarning is raised for case where fval(p0) is already zero where p0 is the initial guess
checks in newton if fval(p0) is already zero, before iterating for any method or evaluating derivatives fprime (or fprime2) if given, and returns _ECONVERGED instead of raising a RuntimeWarning
fails the current test scipy/optimize/tests/test_zeros.py::TestBasic::test_deriv_zero_warning since these two are in opposition.
see post on SciPy Dev mailing list for more info

pvanmulbregt · 2018-06-06T13:35:16Z

scipy/optimize/zeros.py

@@ -187,6 +187,9 @@ def newton(func, x0, fprime=None, args=(), tol=1.48e-8, maxiter=50,
    # Multiply by 1.0 to convert to floating point.  We don't use float(x0)
    # so it still works if x0 is complex.
    p0 = 1.0 * x0
+    # check if initial guess is the solution
+    if func(p0, *args) == 0:
+        return _results_select(full_output, (p0, 0, 0, _ECONVERGED))


The check outside the loop only handles the case of the starting point being a root. Its doesn't handle an intermediate iterate being a multiple root.
Move the test inside the loop, and move the evaluation of f ahead of that of fprime. Something like

for itr in range(maxiter): # First evaluate f fval = func(p0, *args) funcalls += 1 # If fval is 0, a root has been found. Terminate. if fval == 0: return _results_select(full_output, (p0, funcalls, itr, _ECONVERGED)) # Now evaluate f' fder = fprime(p0, *args) funcalls += 1 # If fder is 0 and fval != 0, there is a problem. Warn/terminate if fder == 0: msg = "derivative was zero." warnings.warn(msg, RuntimeWarning) return _results_select(full_output, (p0, funcalls, itr + 1, _ECONVERR)) newton_step = fval / fder ...

I agree, in that case shall I change the title of the PR to "check for convergence using abs of residual" or something like that?

And more importantly, what tolerance should we use to check for convergence in this way? I think a test for zero identically is too strict, perhaps something more like:

# If a root has been found within some tolerance, then terminate if abs(fval) < tol: return _results_select(full_output, (p0, funcalls, itr, _ECONVERGED))

Should we use the same tolerance as what's currently used for the Newton step or add a new argument for a separate one? Or should we just check if the guess is identically zero?

Testing for "exactly 0" fixes the situation of landing in a multiple root.
Testing for "approximately 0" would change existing results. It also isn't scale-invariant (replacing f with const * f leads to different outcomes when using "approximately 0".).
So "exactly 0" is the right criteria to apply.

mikofski · 2018-06-06T14:02:40Z

~~So then is there anyone who is not okay with changing the existing tests?~~ update: apparently not see thread in #8904

* update test to check not just an initial guess at the root but also that if the root is found by checking the residual that it does not raise a RuntimeWarning and that the convergence flag is set * remove test of initial guess before individual methods and iteration loop so that we can check every iteration, don't worry about secant for now * evaluate fval before fder so we can check if it's zero or relative close, use same default residual convergence criteria as the other Zeros (brent, bisect, ridder) * return if residual shows convergence and don't increment iteration count, just function calls

* also change zero residual test to eps too

* use xtol and rtol from zeros

mikofski · 2018-06-07T00:41:07Z

So @pvanmulbregt, is your review affirmative now? @rgommers I believe this fixes the bug #8904 and IMO is an improvement to the existing SciPy NR implementation. Let me know if you have any other comments. Thanks!

rgommers · 2018-06-09T06:08:43Z

scipy/optimize/tests/test_zeros.py

@@ -169,3 +169,36 @@ def f_2(x, *a):
    y = zeros.newton(f, z, args=coeffs, fprime=f_1, fprime2=f_2, tol=1e-6)
    # (-0.75000000000000078+1.1989578808281789j)
    assert_allclose(f(y, *coeffs), 0, atol=1e-6)
+
+
+@pytest.mark.filterwarnings("error")


this line can be removed, this is the default behavior (see pytest.ini line 7 in the repo root).

Hi @rgommers, even though the error warning filter is set by default, it didn't seem to work when I use python runtest.py -t scipy/optimize/tests/test_zeros.py and more importantly I feel that it's more explicit to leave the decorator even if it is redundant.

We want to control this globally. For one, we're switching this behavior between development versions and releases (we do want errors for the former, not for the latter).

rgommers · 2018-06-09T06:09:23Z

scipy/optimize/tests/test_zeros.py

+
+    # a function that has a zero derivative at it's root
+    def f_zeroder_root(x):
+        return x ** 3 - x ** 2


formatting: there should be no spaces around ** (here and below)

rgommers · 2018-06-09T06:09:49Z

scipy/optimize/tests/test_zeros.py

+    r = zeros.newton(f_zeroder_root, x0=0, fprime=fder)
+    assert_allclose(r, 0, atol=zeros._xtol, rtol=zeros._rtol)
+    r = zeros.newton(f_zeroder_root, x0=0, fprime=fder,
+                     fprime2=lambda x: 6 * x - 2)


formatting nitpick: 6*x - 2

rgommers · 2018-06-09T06:13:58Z

Thanks @mikofski. I've made a few minor comments, will be happy to merge once those are addressed.

* conforming to http://pep8.org/#other-recommendations which suggests separating operations with lower priority with spaces

mikofski · 2018-06-14T02:46:16Z

Thanks for your comments @rgommers - hopefully all good now.

rgommers · 2018-06-14T04:31:04Z

Let's give this a go. Thanks @mikofski!

mikofski added 3 commits June 5, 2018 00:51

add test for gh8904

2343ea8

mark gh8904 with warning filter to raise error

aa26183

BUG: check if initial guess is the solution

8f68c09

pvanmulbregt requested changes Jun 6, 2018

View reviewed changes

mikofski mentioned this pull request Jun 6, 2018

BUG: if zero derivative at root, then Newton fails with RuntimeWarning #8904

Closed

mikofski added 7 commits June 6, 2018 11:23

TST: change to nonzero root at x=2, so only test fder is zero

2b1c158

TST: ENH: change test tolerance to eps

a99be87

* also change zero residual test to eps too

TST: ENH: trying to halley to converge

1532ee9

* use xtol and rtol from zeros

TST: ENH: don't use xtol and rtol for now, just test zero indentically

14c1c4e

TST: lower test tolerance for halley

c63b07b

TST: remove last test for halley since fder isn't zero, not right test

3dfb0f8

pvanmulbregt approved these changes Jun 8, 2018

View reviewed changes

mikofski changed the title ~~BUG: ENH: Check if initial guess for newton is already zero~~ BUG: ENH: Check if guess for newton is already zero before checking derivative Jun 8, 2018

rgommers added scipy.optimize maintenance Items related to regular maintenance tasks labels Jun 9, 2018

rgommers reviewed Jun 9, 2018

View reviewed changes

mikofski added 2 commits June 13, 2018 19:34

TST: remove warning filter decorator, remove spaces from exponents

2cd5551

TST: remove whitespace from multiplication

317fd5e

* conforming to http://pep8.org/#other-recommendations which suggests separating operations with lower priority with spaces

rgommers added this to the 1.2.0 milestone Jun 14, 2018

rgommers merged commit 531645f into scipy:master Jun 14, 2018

mikofski deleted the fix_gh8904_zeroder_at_root_newton_fails branch June 14, 2018 08:41

ogrisel mentioned this pull request Dec 1, 2018

[MRG] Add support for classification tasks ogrisel/pygbm#13

Merged

mikofski mentioned this pull request Dec 4, 2018

BUG: scipy.optimize.newton says the root of x^2+1 is zero. #9551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: ENH: Check if guess for newton is already zero before checking derivative #8907

BUG: ENH: Check if guess for newton is already zero before checking derivative #8907

mikofski commented Jun 5, 2018 •

edited

Loading

pvanmulbregt Jun 6, 2018

mikofski Jun 6, 2018 •

edited

Loading

pvanmulbregt Jun 7, 2018

mikofski commented Jun 6, 2018 •

edited

Loading

mikofski commented Jun 7, 2018

rgommers Jun 9, 2018

mikofski Jun 10, 2018

rgommers Jun 11, 2018

mikofski Jun 12, 2018

rgommers Jun 9, 2018

rgommers Jun 9, 2018

rgommers commented Jun 9, 2018

mikofski commented Jun 14, 2018

rgommers commented Jun 14, 2018

BUG: ENH: Check if guess for newton is already zero before checking derivative #8907

BUG: ENH: Check if guess for newton is already zero before checking derivative #8907

Conversation

mikofski commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

mikofski Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski commented Jun 6, 2018 • edited Loading

mikofski commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgommers commented Jun 9, 2018

mikofski commented Jun 14, 2018

rgommers commented Jun 14, 2018

mikofski commented Jun 5, 2018 •

edited

Loading

mikofski Jun 6, 2018 •

edited

Loading

mikofski commented Jun 6, 2018 •

edited

Loading