-
Notifications
You must be signed in to change notification settings - Fork 72
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
Modify constructor to avoid Interval(Inf,Inf) or Interval(-Inf,-Inf) #51
Conversation
I think |
There are some examples in the ITF1788 test suite that go in a slightly different direction. One involves The first one is equivalent to #45. In that case it is correct to have The second one corresponds to |
Ah, I see: |
The case of |
I just noticed some inconsistency; hold on a little bit... |
Actually, the problem of #45 is not the constructor, but |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
+ Coverage 91.64% 91.67% +0.03%
==========================================
Files 21 21
Lines 945 961 +16
==========================================
+ Hits 866 881 +15
- Misses 79 80 +1
Continue to review full report at Codecov.
|
This PR has a problem: The problem lies in this line of a convert method, since |
test/interval_tests/construction.jl
Outdated
@@ -35,6 +35,10 @@ using Base.Test | |||
@test Interval{BigFloat}(1) == Interval{BigFloat}(big(1.0), big(1.0)) | |||
@test Interval{BigFloat}(pi) == Interval{BigFloat}(big(pi), big(pi)) | |||
|
|||
# a < Inf and b > -Inf | |||
@test Interval(Inf, Inf) == Interval(prevfloat(Inf), Inf) |
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 don't understand this line.
Interval(Inf, Inf)
should either return the empty interval or error.
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.
It is an error in the octave interval package.
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 agree it should probably yield an error or an empty interval; however, due to rationalize
in the convert method which is used, we get Interval(Inf,Inf) when it shouldn't. So for the time being I think we should focus in getting around rationalize.
Three tests are skipped since they fail; the rest is passing.
I just pushed another commit (after rebasing to current master). Except for three of our tests, all tests pass! I am skipping checking those cases (a test comparing float intervals with the corresponding rationals, a test involving pi, and a test involving large values and |
src/intervals/conversion.jl
Outdated
end | ||
II | ||
end | ||
convert{T<:AbstractFloat}(::Type{Interval{T}}, x::Float64) = |
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 just far too slow for such a basic operation that is used all over the place, e.g. in Interval(0.1) * 0.3
:
julia> @btime Interval(0.1) * 0.3
700.447 ns (10 allocations: 416 bytes)
Interval(0.03, 0.03000000000000001)
julia> @btime Interval(0.1)
7.612 ns (0 allocations: 0 bytes)
Interval(0.1, 0.1)
julia> @btime convert(Interval{Float64}, 0.3)
610.783 ns (10 allocations: 416 bytes)
Interval(0.3, 0.30000000000000004)
julia> @btime Interval(prevfloat(0.3), nextfloat(0.3))
12.497 ns (0 allocations: 0 bytes)
(A factor of 50 difference!)
I suggest we use parse
for something like convert(Interval, "0.3")
(when a string is given), and simply use convert(::Type{Interval{Float64}, x) = Interval(prevfloat(x), nextfloat(x))
for floats.
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 only thing I did is to eliminate rationalize
and the conditional, so that method directly converts to a string which is parsed and rounded.
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.
Unfortunately the conversion to and from a string is very slow, that's what I'm pointing out. (And probably rationalize
was too.)
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.
Hmm, it seems maybe I'm wrong and this is not the function that is actually used?
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.
No, I'm right.
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.
Rationalize has two problems: sometimes it yields a zero for too small floating points, and sometimes it yields Inf for too large values. I'll see if I can improve the speed...
I think there are two issues in this discussion we should separate: the construction of an interval (which contains whatever checks we wish), and the use of As for the construction, i.e., As you write, we should improve the |
Well, part of the problem as usual is that it is not clear what one means when writing 0.3 -- do you mean the decimal number "0.3", or the floating point number that is printed as 0.3. As far as I can tell, the standard only talks about what to do with strings, in which case certainly we must give the tightest interval, however long that takes: that is, if the user uses a string, it is a sign that (s)he wants the tightest possible interval. With floats there is, as far as I can tell, basically no way of making it performant to get the tightest interval if you interpret 0.3 as "the exact decimal number 0.3". If you interpret 0.3 as "some |
I disagree: the purpose of the standard is to set-up definitions and rules to get the same; returning a tightest interval is part of it. Quoting from the standard (italics are mine): "The aim of the standard is to improve the availability of reliable computing in modern hardware and software environments by defining the basic building blocks needed for performing interval arithmetic. There are presently many systems for interval arithmetic in use; lack of a standard inhibits development, portability, and ability to verify correctness of codes." " This standard specifies: |
The relevant section of the standard is 10.5.8 (pg. 38): there are only two required constructors, namely (in our notation):
(We certainly need to work on the second one for it to be compliant with the standard.) |
Here's another relevant piece (pg. 30): (Italics are mine.) |
All clause 10 is related to Level 1 description, i.e., to the mathematical theory; the level 2 description (finite precision) corresponds to the clause 12. The measures of accuracy is treated in section 12.10. The mode we are discussing is called |
Perhaps we should use traits also for the constructors... |
Yes, that is certainly a possibility. What did you have in mind? Something like
vs
which may not be tight, could work. Or
to use dispatch. |
Also pg. 60: |
Regarding traits, I was thinking in something like Regarding the last quotation you provided, the key part for me is "... in addition to a required “tightest” one,...". That's the reason I think we should focus on this one, and the allow for others. |
Shall we merge this for now, @lbenet? |
I'm not sure why the result for |
Could you give the code sequence that gives |
I agree that
This is my understanding: Yet, this PR is excluding precisely intervals of the form The current result |
The line with temp only ever acts on floats, not on intervals? |
One of the tests that is not passing (due to this PR) is this one. This, I think, is related to the changes in What troubles me is the following: julia> x = 1/3
0.3333333333333333
julia> @interval(x)
Interval(0.33333333333333326, 0.3333333333333333)
julia> big(1)/3 ∈ ans # AUCH!!!
false On the other hand: julia> big(1)/3
3.333333333333333333333333333333333333333333333333333333333333333333333333333348e-01
julia> @interval(1/3)
Interval(0.3333333333333333, 0.33333333333333337)
julia> big(1)/3 ∈ ans
true So rationalize was somehow helping. Any idea? To go back to |
I agree, but it creates an interval; in master we get: julia> x = 2.0^1000
1.0715086071862673e301
julia> x/IntervalArithmetic.half_pi(x)
Interval(Inf, Inf) |
Not at my computer currently but I thought I tried that and did not get that result. That would definitely be a bug, in division apparently? |
i haven't checked the division, but I think the problem is related to #45; somewhere we must rely on |
Ah yes, good catch: since x is divided by an Interval, the first thing that happens is that it gets converted to an Interval. |
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.
Interval(Inf,Inf) is defined to be an error by the standard. Apart from that, this looks good, thanks.
src/intervals/conversion.jl
Outdated
parse(T, string(x), RoundUp)) | ||
end | ||
II | ||
isinf(x) && return Interval{T}(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.
If x is infinite, then it should be an error to convert it to an Interval (though this can happen in the constructor itself).
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.
Good point. Currently this was returning one of the edge intervals, e.g. Interval(realmax(), Inf)
, which was handled at the constructor. The idea to include that here was to skip the rest of the method. (I will change the constructor as you suggest below, and the adapt this.)
src/intervals/intervals.jl
Outdated
# The IEEE Std 1788-2015 states that a < Inf and b > -Inf; | ||
# see page 6, "bounds". | ||
if a == Inf | ||
return new(prevfloat(a), b) |
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.
a == Inf should give an error.
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.
There is a case (cancelminus
I believe) where the subtlety is important.
In any case, I think you are right. I will change this to throw an error (ArgumentError?) and treat the special case separately.
@dpsanders Thanks for the review. I will incorporate the changes. Should I also clean-up (cherry-pick and push -f) this PR? |
… method Currently `@interval(Inf)` returns `Interval(realmax(),Inf)`
Sure, do whatever needs doing. Is |
Pushing what I have now; it's messy (with respect to the end result) but I think it works fine. I want to let travis do its job. |
Tests are passing, including few additions :) So this is a summary of what I did:
The following is a feature: |
Incidentally, |
Only the test introduced in #22 is skipped; the result of |
This is looking very nice, thanks! What was the problem with the power with infinite limits? In my opinion this is ready to merge. If you agree, then please go ahead! |
The problem is that, sometimes, Another comment: as far as I can see, Aside from the last comment, which may be left for another PR, I agree that this should be merged. |
Merging! |
Great, thanks!
…On 10 Jun 2017 9:13 p.m., "Luis Benet" ***@***.***> wrote:
Merged #51
<#51>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALtTrVCFnxWJC-BzVBNcnqKBlpC9E6Vks5sC01KgaJpZM4NzGqE>
.
|
Solves #45.