-
Notifications
You must be signed in to change notification settings - Fork 25
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
Complete rewrite using generic branch and prune with contractors #24
Conversation
Due to the interaction of FowardDiff and StaticArrays. JuliaDiff/ForwardDiff.jl#235
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.
LGTM, and we should merge it, urgently :-)
The question is what to do with the tests. I am aware this is related to other issues (https://github.com/JuliaIntervals/IntervalArithmetic.jl/pull/97/files)
examples/roots.jl
Outdated
|
||
|
||
|
||
h(x) = SVector(2*x - y - exp(-x), -x + 2*y - exp(-y)) |
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.
Running this example throws an error here. I guess you need something like:
h(xv) = ( (x, y) = xv; SVector(2*x - y - exp(-x), -x + 2*y - exp(-y)))
examples/roots.jl
Outdated
h(x) = SVector(2*x - y - exp(-x), -x + 2*y - exp(-y)) | ||
|
||
rts = roots(h, X × X, Bisection) | ||
rts = roots(rts, g, Newton) |
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.
Another error is thrown here. My guess is that this should read as rts = roots(h, rts, Newton)
examples/roots.jl
Outdated
|
||
|
||
# Dennis-Schnabel: | ||
h(x) = SVector(x^2 + y^2 - 2, exp(x - 1) + y^3 - 2) |
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.
I guess you need to correct this function similarly as above.
examples/roots.jl
Outdated
|
||
# Dennis-Schnabel: | ||
h(x) = SVector(x^2 + y^2 - 2, exp(x - 1) + y^3 - 2) | ||
|
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.
The following two lines should read as:
rts = roots(h, X × X, Bisection)
rts = roots(h, rts, Newton)
examples/roots.jl
Outdated
|
||
## MINPACK benchmarks: https://github.com/JuliaNLSolvers/NLsolve.jl/blob/master/test/minpack.jl | ||
|
||
using IntervalArithmetic, IntervalRootFinding, StaticArrays |
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.
This is not needed.
src/branch_and_prune.jl
Outdated
@@ -0,0 +1,205 @@ | |||
using IntervalRootFinding | |||
using IntervalRootFinding: Root |
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.
I think these first two using
clauses are not needed.
src/branch_and_prune.jl
Outdated
Inputs: | ||
- `f`: function whose roots will be found | ||
- `X`: `Interval` or `IntervalBox` | ||
- `contractor`: function that, when applied to the function `f`, determines the status of a given box `X`. It returns the new box and a symbol indicating the status. |
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.
Maybe an example of the contractor
is helpful in the docstrings.
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.
After reading the code, maybe you should specify the possible contractor types.
src/branch_and_prune.jl
Outdated
|
||
|
||
contains_zero{T}(X::Interval{T}) = zero(T) ∈ X | ||
contains_zero(X::SVector) = all(contains_zero(X[i]) for i in 1:length(X)) |
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.
Use here (and below) broadcasting: contains_zero(X::SVector) = all(contains_zero.(X))
src/branch_and_prune.jl
Outdated
end | ||
|
||
|
||
roots{C<:Contractor}(f, X, contractor::Type{C}, tol::Float64=1e-3) = branch_and_prune(f, X, contractor, tol) |
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.
Add docstrings for roots
I have done some work here for tests to pass. Essentially some functions are currently unexported, among other minor things. Shall I commit directly to this branch, or shall I do it in an alternative one? |
Just add it to this huge branch I think. Thanks! |
Just pushed a PR which allows to pass tests locally. If you can take a look on the changes it would be worth doing it, because I think your plan was to deprecate/erase some of the previous functions (e.g., use |
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
=========================================
Coverage ? 78.22%
=========================================
Files ? 7
Lines ? 271
Branches ? 0
=========================================
Hits ? 212
Misses ? 59
Partials ? 0
Continue to review full report at Codecov.
|
Done; tests are passing... |
Thanks for the tests! I have added a basic docstring for roots. I think we should merge this and refine later! |
I converted some of examples.jl into new tests, which pass. |
No description provided.