New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: vectorize scalar zero-search-functions #8357
Changes from 13 commits
c7c5cb8
98672d0
39581f4
f24a559
70edc74
5b90676
44bf2eb
fcca324
5f181cb
cef5de3
c4d6c9f
fe3f1cd
8b7d30e
2f2b3df
9831250
395b046
0b8fe21
25f2f14
4f0bb08
397da36
0866363
ee034e1
3e5d3c9
e0cdc03
5c87dcb
ae9e627
d219ded
1155126
5d3de87
acbc75a
5873bb9
8953401
5166a7f
ffb15bd
4f5ac8a
cfd7571
4373615
5fa936f
cd415e7
95290fb
dc7250f
043ba2e
2792162
86df16a
51b870c
2343ea8
faf14e1
9c242a3
3746e28
c144b11
658dc63
a5af44a
6e083f9
0cfcd3d
45e7b6a
1f2748c
c05a476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,11 @@ | |
import warnings | ||
|
||
from . import _zeros | ||
from numpy import finfo, sign, sqrt | ||
import numpy as np | ||
|
||
_iter = 100 | ||
_xtol = 2e-12 | ||
_rtol = 4*finfo(float).eps | ||
_rtol = 4*np.finfo(float).eps | ||
|
||
__all__ = ['newton', 'bisect', 'ridder', 'brentq', 'brenth'] | ||
|
||
|
@@ -163,54 +163,50 @@ def newton(func, x0, fprime=None, args=(), tol=1.48e-8, maxiter=50, | |
# Newton-Rapheson method | ||
# 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 | ||
fder2 = 0 | ||
p0 = 1.0 * np.asarray(x0) # convert to ndarray | ||
for iter in range(maxiter): | ||
myargs = (p0,) + args | ||
fder = fprime(*myargs) | ||
if fder == 0: | ||
fder = np.asarray(fprime(*myargs)) # convert to ndarray | ||
if (fder == 0).any(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't you need to keep iterating the other elements? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could, but IMO you really don't need to. I think it requires indexing which IMO makes the code a little messy. Do you feel very strongly that it should keep iterating the other cases? I'm open to changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you'd exit early upon hitting a problematic termination condition for any element, even if remaining elements are far from convergence. but I'd defer to a scipy maintainer's opinion here, since non-scalar usage would be new functionality (right? does the released code error on arrays?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. See the traceback in #8354. |
||
msg = "derivative was zero." | ||
warnings.warn(msg, RuntimeWarning) | ||
return p0 | ||
fval = func(*myargs) | ||
if fprime2 is not None: | ||
fder2 = fprime2(*myargs) | ||
if fder2 == 0: | ||
fval = np.asarray(func(*myargs)) # convert to ndarray | ||
if fprime2 is None: | ||
# Newton step | ||
p = p0 - fval / fder | ||
else: | ||
# Parabolic Halley's method | ||
discr = fder ** 2 - 2 * fval * fder2 | ||
if discr < 0: | ||
p = p0 - fder / fder2 | ||
else: | ||
p = p0 - 2*fval / (fder + sign(fder) * sqrt(discr)) | ||
if abs(p - p0) < tol: | ||
fder2 = np.asarray(fprime2(*myargs)) # convert to ndarray | ||
# Halley's method | ||
# https://en.wikipedia.org/wiki/Halley%27s_method | ||
p = p0 - 2 * fval * fder / (2 * fder ** 2 - fval * fder2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is removing the switch changing the scalar behavior here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this switch is particular to the "parabolic Halley's method" a variant, see link in #5922. I removed it because branches require indexing which IMO makes the code a little messy. Using the traditional Halley's method, see Wikipedia link in code, removed the need for branching, and IMHO makes the code cleaner. Does this answer your question? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. Guess I'd avoid coupling behavior changes for the scalar case to the feature addition of vectorization (but again my opinion here matters less than the maintainers'). The two changes could be done separately, one but not the other, or both, though vectorizing the modified version is simpler as you say. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree 100% with this. Let's stick to one issue at a time. (The modification looks more prone to overflow because of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current version (as of scipy-1.0.0) with the "parabolic" variant of Halley's already has I agree it would be more explicit to future maintainers to understand if we separated the switch from the parabolic variant of Halley's to the more widely accepted form into a separate PR. Although it's a requirement for this PR, because the more common version of Halley's doesn't require any branching which I'm really hoping to avoid here. So should I open another PR to propose switching out the parabolic variant of Halley's? |
||
if np.abs(p - p0).max() < tol: | ||
return p | ||
p0 = p | ||
else: | ||
# Secant method | ||
p0 = x0 | ||
if x0 >= 0: | ||
p1 = x0*(1 + 1e-4) + 1e-4 | ||
else: | ||
p1 = x0*(1 + 1e-4) - 1e-4 | ||
q0 = func(*((p0,) + args)) | ||
q1 = func(*((p1,) + args)) | ||
p0 = np.asarray(x0) | ||
dx = np.finfo(float).eps**0.33 | ||
dp = np.where(p0 >= 0, dx, -dx) | ||
p1 = p0 * (1 + dx) + dp | ||
q0 = np.asarray(func(*((p0,) + args))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same as the much more readable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you so much for your thorough and thoughtful feedback! I agree 100% - this snippet is from the original scalar newton code - I was trying to make the minimum amount of changes possible, but I agree this snippet could be improved to be more readable. I've made a new issue #8589 to address readability in both the original newton method and the proposed array newton method. |
||
q1 = np.asarray(func(*((p1,) + args))) | ||
for iter in range(maxiter): | ||
if q1 == q0: | ||
if p1 != p0: | ||
msg = "Tolerance of %s reached" % (p1 - p0) | ||
divide_by_zero = (q1 == q0) | ||
if divide_by_zero.any(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here as well, wouldn't it be better to continue for all other elements? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my previous answer in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The halting conditions are the biggest thing to think about here. This way is simple, but I'm sure that someone is going to yell at us in the future about evaluating their callback function too many times. Might be worth seeing how messy the indexing would look. |
||
tolerance_reached = (p1 != p0) | ||
if (divide_by_zero & tolerance_reached).any(): | ||
msg = "Tolerance of %s reached" % np.sqrt(sum((p1 - p0)**2)) | ||
warnings.warn(msg, RuntimeWarning) | ||
return (p1 + p0)/2.0 | ||
else: | ||
p = p1 - q1*(p1 - p0)/(q1 - q0) | ||
if abs(p - p1) < tol: | ||
if np.abs(p - p1).max() < tol: | ||
return p | ||
p0 = p1 | ||
q0 = q1 | ||
p1 = p | ||
q1 = func(*((p1,) + args)) | ||
q1 = np.asarray(func(*((p1,) + args))) | ||
msg = "Failed to converge after %d iterations, value is %s" % (maxiter, p) | ||
raise RuntimeError(msg) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two lines could be