Skip to content
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

Differences in brent implementation of brent.jl and brentmin.jl #1036

Open
kaber2 opened this issue Apr 3, 2023 · 2 comments
Open

Differences in brent implementation of brent.jl and brentmin.jl #1036

kaber2 opened this issue Apr 3, 2023 · 2 comments

Comments

@kaber2
Copy link

kaber2 commented Apr 3, 2023

Hello,

I was comparing the implementations of:

And notices a few differences. I was wondering if someone can shed some light on which of the two is correct.

First, while brent.jl uses:

    if abs(new_minimizer - x_midpoint) <= 2*x_tol - (x_upper-x_lower)/2

As stopping criterium, brentmin.hl has the second condition turned around:

    if abs(x - m) > 2 * tol - (b - a) / 2 # stopping crit

(b-a instead of a-b). A quick test in my own implementation shows that brentmin.jl seems to be incorrect as even simple functions don't converge anymore.

Then, when selecting the new step, brent.jl does:

        new_x = new_minimizer + ((step > 0) ? x_tol : -x_tol)

while brentmin.jl dies:

            u = x + sign(d) * tol

So it treats the case of d == 0 differently.

@pkofod
Copy link
Member

pkofod commented Apr 11, 2023

Both are correct, the conditions are turned around because in Optim.jl the code path with the condition you mention stops if true and in NLSolvers.jl the code paths continues if true, hence the swapped inequality. The reference implementation in Brent's book has it the same way NLSolvers.jl does

image

So if that condition is true (no convergence) it continues.

As per the second part, that is true, it should be copysign(tol, d) as per brent's book
image

@pkofod
Copy link
Member

pkofod commented Apr 11, 2023

Please provide the simple functions you're referring to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants