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

Use branch and prune as backend for the search #187

Merged
merged 3 commits into from
Feb 19, 2023
Merged

Conversation

Kolaru
Copy link
Collaborator

@Kolaru Kolaru commented Feb 13, 2023

Replaces the internal branch and prune search by BranchAndPrune.jl v0.2.

This is the first of 3 planned PR

  1. Use BranchAndPrune package
  2. Refactor contractors to dissociate them totally from the iteration configuration (only the absolute tolerance for now)
  3. Refactor the whole interface, using a RootProblem struct for better handling of iteration config, and add options like max iteration and relative tolerance

@Kolaru
Copy link
Collaborator Author

Kolaru commented Feb 17, 2023

@lucaferranti @lbenet @dpsanders If there is no strong feeling against this, I'd like to proceed in the coming days.

@lucaferranti
Copy link
Member

Sorry! overall looks good. I'll give a more detailed review during the weekend. If I faik to review by monday, feel free to merge then

end

const NewtonLike = Union{Type{Newton}, Type{Krawczyk}}
const default_strategy = DepthFirstSearch
const default_search_order = DepthFirst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, do you have any benchmarks / insights / literature references on how the two search strategies compare for interval root finding? I guess they are pretty much equivalent and there's not a strong advantage of one over the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the main difference is that is you you have a limited number of iteration, going breadth first give you evenly sized intervals over the search domain, while going depth first gives you smallest possible intervals for the subset of the search domain that it had time to go through.

@dpsanders
Copy link
Member

In general I think this is a great idea, but it would be good to benchmark to make sure that performance is OK - at some point I remember comparing with a simple implementation which was considerably faster.

I seem to remember that there is a separate implementation of Newton in 1D that it would be useful to keep.

@lbenet
Copy link
Member

lbenet commented Feb 18, 2023

I seem to remember that there is a separate implementation of Newton in 1D that it would be useful to keep.

It is still there: src/newton1d.jl.

Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Go ahead and merge!

@Kolaru
Copy link
Collaborator Author

Kolaru commented Feb 19, 2023

In general I think this is a great idea, but it would be good to benchmark to make sure that performance is OK - at some point I remember comparing with a simple implementation which was considerably faster.

To get a bit of peace of mind I quickly benchmarked the general method versus newton1d and it is seems to be faring pretty well (the previous implementation not using branch and prune is on a quite old version). newton1d do some additional checks, so it is not totally surprising it is slower.

julia> f(x) = x^2 - 2
f (generic function with 1 method)

julia> df(x) = 2x
df (generic function with 1 method)

julia> abstol = 1e-7
1.0e-7

julia> X = -5..5
[-5, 5]

julia> @benchmark newton1d(f, df, X ; abstol)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  166.700 μs   15.693 ms  ┊ GC (min  max): 0.00%  73.56%
 Time  (median):     175.000 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   214.903 μs ± 588.151 μs  ┊ GC (mean ± σ):  8.09% ±  2.92%

  ▇█▇▅▄▄▄▂▄▃▃▃▂▁▁▁▁                                             ▂
  █████████████████████▇█▇██▇▇█▇▇▇▇▇▇▇▇▇▇▆▆▆▆▆▆▆▆▅▄▆▅▆▆▄▆▆▄▄▄▄▄ █
  167 μs        Histogram: log(frequency) by time        402 μs <

 Memory estimate: 86.91 KiB, allocs estimate: 2086.

julia> @benchmark roots(f, df, X, Newton, abstol)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  137.800 μs   17.256 ms  ┊ GC (min  max): 0.00%  78.47%
 Time  (median):     146.900 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   179.884 μs ± 568.193 μs  ┊ GC (mean ± σ):  8.74% ±  2.74%

  ██▇▆▆▅▅▃▃▃▃▂▂▁  ▁                                             ▂
  ████████████████████▇▇▇▇▇▇▇▆▇▆▇▇▇▇▇▆▆▆▅▅▅▆▅▅▅▆▅▆▆▅▅▅▅▅▅▆▅▄▄▅▄ █
  138 μs        Histogram: log(frequency) by time        359 μs <

 Memory estimate: 73.88 KiB, allocs estimate: 1774.

@Kolaru Kolaru merged commit 544b635 into master Feb 19, 2023
@Kolaru Kolaru deleted the branch_and_prunev0.2 branch March 4, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants