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

Add methods for IntervalBox previously in IntervalRootFinding #160

Merged
merged 4 commits into from
Jun 24, 2018

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Jun 19, 2018

Fixes #158

@lbenet
Copy link
Member Author

lbenet commented Jun 19, 2018

See also JuliaIntervals/IntervalRootFinding.jl#78, which requires a new patch version.

@lbenet
Copy link
Member Author

lbenet commented Jun 19, 2018

@dpsanders Question: Currently, mid(X) is equivalent to mid(X, 0.5); on the other hand, bisect(X) corresponds to bisect(X, 127/256), which was the way it was defined in IntervalRootFinding. Shall I leave it like that, or have the default value equal to 0.5, and change things in IntervalRootFinding?

Incidentally, tests are passing (except maybe for appveyor).

@codecov-io
Copy link

codecov-io commented Jun 19, 2018

Codecov Report

Merging #160 into master will increase coverage by 0.2%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #160     +/-   ##
=========================================
+ Coverage   91.95%   92.15%   +0.2%     
=========================================
  Files          22       23      +1     
  Lines        1044     1058     +14     
=========================================
+ Hits          960      975     +15     
+ Misses         84       83      -1
Impacted Files Coverage Δ
src/IntervalArithmetic.jl 100% <ø> (ø) ⬆️
src/multidim/intervalbox.jl 78.57% <0%> (-9.43%) ⬇️
src/bisect.jl 100% <100%> (ø)
src/intervals/intervals.jl 80.95% <100%> (+0.46%) ⬆️
src/intervals/special.jl 100% <100%> (+9.09%) ⬆️
src/intervals/arithmetic.jl 97.43% <0%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f399c8...dc8db55. Read the comment docs.

@lbenet
Copy link
Member Author

lbenet commented Jun 22, 2018

@dpsanders Should I go ahead and merge this?

@lbenet lbenet mentioned this pull request Jun 22, 2018
@dpsanders
Copy link
Member

The default for mid corresponds to that of the IEEE standard, so I guess we should leave it.
The default for bisect is more useful as being not equal to 0.5, since that avoids problems with zeros at 0 when using symmetric intervals, and generally seems more useful.

@@ -45,7 +45,7 @@ Return a vector of the `mid` of each interval composing the `IntervalBox`.
See `mid(X::Interval, α=0.5)` for more informations.
"""
mid(X::IntervalBox) = mid.(X)
mid(X::IntervalBox, α) = mid.(X, α)
mid(X::IntervalBox, α) = mid.(X[:], α)
Copy link
Member

Choose a reason for hiding this comment

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

This can be left as mid.(X, alpha) after incorporating the change in #159

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall I wait that #159 to change it, or should I leave it like this for the time being? (#161 has some difficulties related to this.)

@@ -73,6 +73,12 @@ diam(X::IntervalBox) = maximum(diam.(X.v))

emptyinterval(X::IntervalBox{N,T}) where {N,T} = IntervalBox(emptyinterval.(X.v))

isinf(X::IntervalBox) = any(isinf.(X))
Copy link
Member

Choose a reason for hiding this comment

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

This can be written any(isinf, X), which I believe should be slightly faster.


isinterior(X::IntervalBox{N,T}, Y::IntervalBox{N,T}) where {N,T} = all(isinterior.(X, Y))

contains_zero(X::SVector) = all(contains_zero.(X))
Copy link
Member

Choose a reason for hiding this comment

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

all(contains_zero, X)

isinterior(X::IntervalBox{N,T}, Y::IntervalBox{N,T}) where {N,T} = all(isinterior.(X, Y))

contains_zero(X::SVector) = all(contains_zero.(X))
contains_zero(X::IntervalBox) = all(contains_zero.(X))
Copy link
Member

Choose a reason for hiding this comment

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

Same

@@ -14,7 +14,6 @@ end

@testset "Complex functions" begin
Z = (3 ± 1e-7) + (4 ± 1e-7)*im
@test sin(Z) == Interval(3.853734949309744, 3.8537411265295543) - Interval(27.016810169394066, 27.016816346613904)*im

@test sin(Z) == complex(sin(real(Z))*cosh(imag(Z)),sinh(imag(Z))*cos(real(Z)))
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I though this way is more stable :-)

@dpsanders
Copy link
Member

LGTM, thanks! Feel free to incorporate #159 here.

@lbenet
Copy link
Member Author

lbenet commented Jun 23, 2018

I've merged #159, rebased to current master, included a missing suggestion of your review. I'll wait for travis tests to pass, and then merge.

@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage increased (+0.2%) to 92.155% when pulling dc8db55 on lb/iss158 into 8f399c8 on master.

@lbenet lbenet merged commit 37f126e into master Jun 24, 2018
@lbenet
Copy link
Member Author

lbenet commented Jun 24, 2018

Merged.

@@ -0,0 +1,42 @@

const where_bisect = 0.49609375

Choose a reason for hiding this comment

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

This looks like a magic constant :) 0.5 - 1/256, if I'm not mistaken. Is there a particular reason for that?

@dpsanders
Copy link
Member

No particular reason, just something close to 0.5 but not exactly equal to 0.5.

@lbenet
Copy link
Member Author

lbenet commented Nov 26, 2018

I think we needed something close to zero and to be exactly-representable floating point. Also, I think there were cases were rounding down had some sort of preference.

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.

5 participants