Add support for Infinity.jl #95

fchorney · 2020-05-27T20:26:35Z

Using #92 as a jump off point. This is a PR that utilizing Infinity.jl instead of using nothing as a sentinel value to represent unbounded ranges.

There were still a few changes and fixes to be made, but I think this somewhat just works(tm).

If this is good, the next steps would be to add Date/Time support to Infinity.jl

Most of this PR is just adding tests

fchorney · 2020-05-27T20:28:43Z

test/comparisons.jl

+            @show a
+            @show b
+            earlier = convert(A, Interval(a[1], a[2], a[3], a[4]))
+            later = convert(B, Interval(b[1], b[2], b[3], b[4]))


These converts seem to fail for AnchoredIntervals, and I currently can't seem to figure out how to fix it.

Seems to fail here https://github.com/invenia/Intervals.jl/blob/master/src/anchoredinterval.jl#L165 with the error

a = (-∞, 2, true, true) b = (4, ∞, true, true) non-overlapping: Error During Test at /Users/fchorney/Documents/repos/julia/Intervals.jl/test/comparisons.jl:20 Got exception outside of a @test TypeError: in Type, in parameter, expected Int64, got InfExtended{Int64} Stacktrace: [1] convert(::Type{AnchoredInterval{Intervals.Direction{:Right}(),T} where T}, ::Interval{InfExtended{Int64}}) at /Users/fchorney/Documents/repos/julia/Intervals.jl/src/anchoredinterval.jl:167 [2] top-level scope at /Users/fchorney/Documents/repos/julia/Intervals.jl/test/comparisons.jl:29 [3] top-level scope at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1107 [4] top-level scope at /Users/fchorney/Documents/repos/julia/Intervals.jl/test/comparisons.jl:21 [5] top-level scope at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1180 [6] include at ./boot.jl:328 [inlined] [7] include_relative(::Module, ::String) at ./loading.jl:1105 [8] include(::Module, ::String) at ./Base.jl:31 [9] include(::String) at ./client.jl:424 [10] top-level scope at /Users/fchorney/Documents/repos/julia/Intervals.jl/test/runtests.jl:15 [11] top-level scope at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1107 [12] top-level scope at /Users/fchorney/Documents/repos/julia/Intervals.jl/test/runtests.jl:15 [13] include at ./boot.jl:328 [inlined] [14] include_relative(::Module, ::String) at ./loading.jl:1105 [15] include(::Module, ::String) at ./Base.jl:31 [16] include(::String) at ./client.jl:424 [17] top-level scope at none:6 [18] eval(::Module, ::Any) at ./boot.jl:330 [19] exec_options(::Base.JLOptions) at ./client.jl:263 [20] _start() at ./client.jl:460

I think this call is occuring convert(AnchoredInterval{Ending}, Interval(4, ∞, true, true)) which doesn't make sense.

I’m unsure why that doesn’t make any sense? Seems like it would work how it used to but now has infinity values in it

The reason it's non-sense is the point at which you're setting the one endpoint used in the AnchoredInterval is ∞. Then we're calculating the other endpoint by subtracting 4 from ∞ which is also ∞. There doesn't seem to be a way to do this conversion without losing information which is why I'm thinking this conversion should error. We should detect this and report a nice error.

I wrote up another example here: #89 (comment)

ah ok I see. Thanks for the explanation!

omus · 2020-05-27T20:35:30Z

src/interval.jl

+        end
+        return Interval(f, l, inc)
+    end
+end


I think if a user has explicitly set T we shouldn't be messing with it.

hm yeah I suppose this change is actually just a byproduct of trying to throw in unbounded values into the tests.

This test is specifically the issue (https://github.com/invenia/Intervals.jl/blob/fc/using-infinity/test/interval.jl#L46-L47):

@test Interval(b, a, Inclusivity(true, false)) == Interval{typeof(a)}(a, b, Inclusivity(false, true))

When you create the interval on the left side of == it will promote(b, a) so if one value is ∞ and the other is a value, let's say 10, then it converts both values to InfExtended. Now on the right side of ==, if a is of type Infinite (due to being a ∞ or -∞) then it will try to convert(Infinite, 10) for b, which is an error. On the other side of the spectrum, if a is 10 which would make it Int64 type, then trying to do convert(Int64, ∞) for b will also fail. I guess we can just let it be an error if somebody puts in the wrong type in the Interval constructor.

I think the test should be updated to be using:

julia> promote_type(Infinite, Int) InfExtended{Int64}

Then in the tests you can use Interval{promote_type(typeof(a), typeof(b))}(...)

omus · 2020-05-27T20:37:09Z

src/interval.jl

@@ -161,7 +181,8 @@ end

 ##### ARITHMETIC #####

-Base.:+(a::T, b) where {T <: Interval} = T(first(a) + b, last(a) + b, inclusivity(a))
+Base.:+(a::Interval, b) = Interval(first(a) + b, last(a) + b, inclusivity(a))


The previous version always kept the Interval unit type the same. This could cause the Interval type to change.

This becomes the same issue as most of the other issues that cropped up by adding Infinity. If a is -∞ .. ∞ and b is of type Int then ∞ - b where b is 1 becomes an InfExtended{Int64} type which will error out since T would have been Interval{Infinite}(...). So we end up trying to run convert(Infinite, InfExtended{Int64}) which is undefined. That being said, maybe that shouldn't be undefined. Converting an InfExtended{Int64}) to Infinite could potentially work if we check the value to make sure it's not an actual extended value

That conversion makes sense if the InfExtended{Int64} is infinite

Fixed by adding the conversion: cjdoris/Infinity.jl#7

omus · 2020-05-27T20:38:30Z

src/interval.jl

+    return Interval(
+        promote(left.endpoint, right.endpoint)...,
+        left.included, right.included
+    )


Seems preferable to change the endpoint constructor (Interval{T}(left::LeftEndpoint{T}, right::RightEndpoint{T}))

The problem here is that you'll end up with situations such that

typeof(left) = Intervals.Endpoint{InfExtended{Int64},Intervals.Direction{:Left}()} typeof(right) = Intervals.Endpoint{Int64,Intervals.Direction{:Right}()}

Thus you don't actually make it to that constructor you mention since the types are different. It ends up trying to call Interval(f, l, inc...) = Interval(promote(f, l)..., inc...) which fails because there are no rules on how to promote two endpoints.

Would it make sense to also define a constructor for two endpoints where the types are different? I seems like we typically don't want to change what the Interval Type is, but with Infinity it seems to be required in certain situations

function Interval(left::LeftEndpoint{S}, right::RightEndpoint{D}) where {S, D} T = promote_type(S, D) return Interval{T}(left.endpoint, right.endpoint, left.included, right.included) end

That is one option. We may alternatively want to define our own promote_rule

omus · 2020-05-27T20:42:12Z

test/comparisons.jl

+        tests = [
+            ((1, 2, true, true), (4, 5, true, true)),
+            ((-∞, 2, true, true), (4, ∞, true, true)),
+        ]


Testsets have a for-loop syntax which would be better as you can output details in the name of the testset (https://docs.julialang.org/en/v1/stdlib/Test/#Test.@testset). I'd probably prefer a distinct testset named "non-overlapping unbounded"

omus · 2020-05-27T20:48:34Z

test/comparisons.jl

+            @show a
+            @show b
+            earlier = convert(A, Interval(a[1], a[2], a[3], a[4]))
+            later = convert(B, Interval(b[1], b[2], b[3], b[4]))


I think this call is occuring convert(AnchoredInterval{Ending}, Interval(4, ∞, true, true)) which doesn't make sense.

src/interval.jl

omus · 2020-05-27T20:50:38Z

src/Intervals.jl

@@ -1,6 +1,8 @@
 module Intervals

 using Dates
+using Infinity


Is this needed as you explicitly import methods below?

If you don’t then you can’t use the infinity symbol correctly. I suppose I could just import it with the other items.

In the context of the package I don't think we're referencing the infinity symbol

hmm good point I guess it's just for the tests.

omus · 2020-05-27T20:54:12Z

test/interval.jl

+        @test Interval(-∞, ∞) == Interval(-∞, ∞)
+        @test Interval(-∞, ∞) != Interval(-∞, ∞, true, false)
+        @test Interval(-∞, ∞) != Interval(-∞, ∞, false, true)
+        @test Interval(-∞, ∞) != Interval(-∞, ∞, false, false)


I thought we weren't allowing inclusive endpoints for unbounded endpoints?

I wasn’t sure if we still wanted to do that with proper infinity. As it potentially makes sense for infinity itself to be inclusive or not? It definitely makes the logic nicer to include inclusivity. I am willing to go back to mutating the inclusivity.

As infinity is the first non-finite value we're supporting in intervals I think there is going to be some specialized logic around it. I think I'll write some of the logic down in the original issue.

After writing down specific rules I think we can leave this as it is: #89 (comment)

codecov · 2020-05-28T21:29:50Z

Codecov Report

Merging #95 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   96.49%   96.57%   +0.07%     
==========================================
  Files           7        7              
  Lines         257      321      +64     
==========================================
+ Hits          248      310      +62     
- Misses          9       11       +2

Impacted Files	Coverage Δ
src/Intervals.jl	`100.00% <ø> (ø)`
src/anchoredinterval.jl	`95.49% <100.00%> (+0.66%)`	⬆️
src/interval.jl	`97.34% <100.00%> (+0.28%)`	⬆️

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 5c2f410...aba26a2. Read the comment docs.

omus · 2020-06-01T18:01:53Z

src/interval.jl

+function isbounded(a)
+    T = typeof(a)
+    if T <: Infinite || T <: InfExtended
+        return !isposinf(a) && !isneginf(a)
+    else
+        return true
+    end
+end


isfinite works with Infinity and InfExtended types as well as the Inf from Float64. Additionally this works with TimeTypes:

Suggested change

function isbounded(a)

T = typeof(a)

if T <: Infinite || T <: InfExtended

return !isposinf(a) && !isneginf(a)

else

return true

end

end

isbounded(x) = isfinite(x)

omus · 2020-06-01T19:54:01Z

We were aware that AnchoredIntervals could only support representing an unbounded interval where the finite point would be stored. Since an AnchoredInterval uses a single point and a span to represent a range it is only possibly to represent a range in this form if we use finite point (due to infinity + finite = infinity).

However there is currently another problem:

julia> convert(AnchoredInterval{Beginning}, Interval(4, ∞, true, true))
ERROR: TypeError: in Type, in parameter, expected Int64, got InfExtended{Int64}
Stacktrace:
 [1] convert(::Type{AnchoredInterval{Intervals.Direction{:Left}(),T} where T}, ::Interval{InfExtended{Int64}}) at /Users/omus/.julia/dev/Intervals/src/anchoredinterval.jl:169
 [2] top-level scope at REPL[4]:1

This problem is due to isbits(InfExtended{Int64}(∞)) being false:

julia> AnchoredInterval{InfExtended{Int64}(∞),InfExtended{Int64}}(4,Inclusivity(true,true))
ERROR: TypeError: in Type, in parameter, expected Int64, got InfExtended{Int64}
Stacktrace:
 [1] top-level scope at REPL[5]:1

julia> AnchoredInterval{∞,InfExtended{Int64}}(4,Inclusivity(true,true))
AnchoredInterval{∞,InfExtended{Int64}}(4, Inclusivity(true, true))

See: cjdoris/Infinity.jl#4

omus · 2020-06-01T20:21:30Z

Solutions:

Make InfExtended{T} a bitstype which allows values of this type to be used for parametric structs (InfExtended is not a bitstype cjdoris/Infinity.jl#4)
Eliminate using span values in the type parameter (Move away from using span value as a type parameter #12)

Workarounds:

Ignore/remove unbounded support for AnchoredIntervals
Special case conversion to change the infinite span to be of type Infinite rather than InfExtended{T} as Infinite happens to be a bitstype

Prevents it from being used in type parameters

Allows for use of types where zero cannot be represented. Note that using `signbit(x)` appears preferable to `sign(x) < 0` but unfortunately less types support `signbit` (e.g. Date)

omus · 2020-08-14T16:36:58Z

Closing this PR as unbounded support was added in #104 and support for Infinity.jl was added in #107

Try using Infinity

617d1e1

fchorney requested a review from omus as a code owner May 27, 2020 20:26

fchorney commented May 27, 2020

View reviewed changes

Add support for Infinity.jl

4e3ebe5

fchorney force-pushed the fc/using-infinity branch from b8bc4e0 to 4e3ebe5 Compare May 27, 2020 20:32

omus reviewed May 27, 2020

View reviewed changes

fchorney added 4 commits May 28, 2020 15:30

Revert the comparisons.jl test changes

6b81961

Revert constructor change. Fix up tests

52c528a

Clean up constructor tests

f50c266

Fix Interval constructor for two endpoints

3564a85

omus reviewed Jun 1, 2020

View reviewed changes

omus added 6 commits June 2, 2020 13:06

Use isfinite for bounded check

2e1a039

Improve generality of comparison tests

614967f

Experiment with unbounded intervals in comparisons

945757d

Work around InfExtended{T} not being bits

dfb91a7

Prevents it from being used in type parameters

Detect negative via sign instead of zero

f8d3231

Allows for use of types where zero cannot be represented. Note that using `signbit(x)` appears preferable to `sign(x) < 0` but unfortunately less types support `signbit` (e.g. Date)

Update comparisons test with unbounded intervals

aba26a2

omus mentioned this pull request Jun 3, 2020

Support intervals endpoints using infinity #99

Merged

omus closed this Aug 14, 2020

omus deleted the fc/using-infinity branch August 14, 2020 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Infinity.jl #95

Add support for Infinity.jl #95

fchorney commented May 27, 2020 •

edited

Loading

fchorney May 27, 2020

omus May 27, 2020

fchorney May 27, 2020

omus May 28, 2020

omus May 28, 2020

fchorney May 28, 2020

omus May 27, 2020

fchorney May 28, 2020 •

edited

Loading

omus May 28, 2020

omus May 27, 2020

fchorney May 28, 2020

omus Jun 2, 2020

omus Jun 3, 2020

omus May 27, 2020

fchorney May 28, 2020

fchorney May 28, 2020

omus Jun 2, 2020

omus May 27, 2020

omus May 27, 2020

omus May 27, 2020

fchorney May 27, 2020

omus May 28, 2020

fchorney May 28, 2020

omus May 27, 2020

fchorney May 27, 2020

omus May 28, 2020

omus May 28, 2020

codecov bot commented May 28, 2020 •

edited

Loading

omus Jun 1, 2020

omus commented Jun 1, 2020 •

edited

Loading

omus commented Jun 1, 2020 •

edited

Loading

omus commented Aug 14, 2020

Add support for Infinity.jl #95

Add support for Infinity.jl #95

Conversation

fchorney commented May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchorney May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 28, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

omus commented Jun 1, 2020 • edited Loading

omus commented Jun 1, 2020 • edited Loading

omus commented Aug 14, 2020

fchorney commented May 27, 2020 •

edited

Loading

fchorney May 28, 2020 •

edited

Loading

codecov bot commented May 28, 2020 •

edited

Loading

omus commented Jun 1, 2020 •

edited

Loading

omus commented Jun 1, 2020 •

edited

Loading