Add unbounded endpoints. Represented by `nothing` #92

fchorney · 2020-05-25T20:16:42Z

Closes #89

Initial WIP for adding unbounded endpoints which will be represented by nothing

There seems to be a lot of edge cases to take into account. There are still a lot of tests that need to be written, and more modifications to support unbounded intervals (such as intersection, union, etc.

omus · 2020-05-26T13:18:59Z

There are still a lot of tests that need to be written

I highly recommend you look at test/comparisons.jl for this. If you add all combinations of unbounded ranges to these tests you'll be in a really good state. You may not even need to write any tests beyond that

omus · 2020-05-26T13:23:33Z

src/endpoint.jl

-    a.endpoint < b.endpoint || (a.endpoint == b.endpoint && a.included && !b.included)
+    if isnothing(a.endpoint) && isnothing(b.endpoint)
+        return false
+    end


If a is unbounded and b is not then a < b

omus · 2020-05-26T13:25:36Z

src/endpoint.jl

@@ -7,10 +7,10 @@ const Beginning = Left
 const Ending = Right

 struct Endpoint{T, D}
-    endpoint::T
+    endpoint::Union{T, Nothing}


For Endpoint I could see not making the endpoint field a Union. What I'm imagining is that you'd have Endpoint{Nothing, Left} for an unbounded left side. The advantage this may have is you can use multiple dispatch to special case the unbounded endpoint comparisons

omus · 2020-05-26T13:27:12Z

src/endpoint.jl

+    if isnothing(a.endpoint) && isnothing(b.endpoint)
+        return false
+    end
+    return a.endpoint < b.endpoint


return isnothing(a.endpoint) || isnothing(b.endpoint) || a.endpoint < b.endpoint

omus · 2020-05-26T13:28:30Z

src/endpoint.jl

+        return false
+    else
+        return a.endpoint < b.endpoint || (a.endpoint == b.endpoint && !(a.included && b.included))
+    end


return !isnothing(a.endpoint) && !isnothing(b.endpoint) && (...)

omus · 2020-05-26T13:29:01Z

src/endpoint.jl

-Base.:(==)(a, b::Endpoint) = a == b.endpoint && b.included
+function Base.:(==)(a, b::Endpoint)
+    a == b.endpoint && b.included
+end


Why the change to the long-function form?

I think I was going to add something in here, but then found the actual place I had to change. Will revert

omus · 2020-05-26T14:11:06Z

src/endpoint.jl

+    end
+end
+# If the right endpoint is unbounded, then it is always greater than any other value
+Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) ? true : a < b.endpoint


Suggested change

Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) ? true : a < b.endpoint

Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) || a < b.endpoint

omus · 2020-05-26T14:11:46Z

src/endpoint.jl

+# If the right endpoint is unbounded, then it is always greater than any other value
+Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) ? true : a < b.endpoint
+# If the left endpoint is unbounded, then it is always less than any other value
+Base.isless(a::LeftEndpoint, b) = isnothing(a.endpoint) ? true : a.endpoint < b


Suggested change

Base.isless(a::LeftEndpoint, b) = isnothing(a.endpoint) ? true : a.endpoint < b

Base.isless(a::LeftEndpoint, b) = isnothing(a.endpoint) || a.endpoint < b

omus · 2020-05-26T14:18:41Z

src/interval.jl

+    if isnothing(f) || isnothing(l)
+        # If only one endpoint is nothing, use the type of the other endpoint
+        T = isnothing(f) ? typeof(l) : typeof(f)
+        return Interval{T}(f, l, inc...)
+    else
+        return Interval(promote(f, l)..., inc...)
+    end


Use more dispatch:

Interval(f::Nothing, l::T, inc...) where T = Interval{T}(f, l, inc...) Interval(f::T, l::Nothing, inc...) where T = Interval{T}(f, l, inc...)

omus · 2020-05-26T14:20:30Z

src/interval.jl

+        return false
+    else
+        return re < le
+    end


Logic shouldn't have to change here as endpoint comparisons should handle unbounded right?

I'll have to double check. At some point I found that some of the tests were failing here without modifications, but I'll go back to it and make sure

omus · 2020-05-26T14:21:26Z

src/interval.jl

+        end
+    else
+        return !(a ≫ b || a ≪ b)
+    end


I would expect there should be no changes here and this logic would be handled by less_than_disjoint

nickrobinson251 · 2020-05-26T17:50:31Z

src/endpoint.jl

@@ -80,26 +80,59 @@ function Base.isequal(a::RightEndpoint, b::LeftEndpoint)
 end

 function Base.isless(a::LeftEndpoint, b::LeftEndpoint)
-    a.endpoint < b.endpoint || (a.endpoint == b.endpoint && a.included && !b.included)
+    if isnothing(a.endpoint) && isnothing(b.endpoint)


all of the isnothing(x)s should x === nothing for reasons i don't pretend to understand but which are referenced in https://docs.julialang.org/en/v1/manual/performance-tips/#Checking-for-equality-with-a-singleton-1 (xref JuliaLang/julia#35585)

That's true. We may want to have a isunbounded abstraction as we may switch our internal representation

ah interesting. Will do. I had read somewhere to use isnothing(x) but that makes sense

Actually, I believe isnothing(x) should be as performant as x === nothing it's x == nothing that can be slightly slower

I like the abstraction idea. I'll throw that in as well

fchorney · 2020-05-26T21:45:55Z

src/Intervals.jl

@@ -41,5 +41,6 @@ export AbstractInterval,
       less_than_disjoint,
       greater_than_disjoint,
       superset,
-       .., ≪, ≫, ⊆, ⊇, ⊈, ⊉
+       .., ≪, ≫, ⊆, ⊇, ⊈, ⊉,
+       Endpoint, Left, Right, LeftEndpoint, RightEndpoint


I added this for testing purposes. I will need to remove this later.

fchorney · 2020-05-26T21:46:45Z

src/endpoint.jl

+
+# Note: Are these two comparisons correct?
+Base.isless(a::Endpoint{Nothing}, b::Endpoint{T}) where T = true
+Base.isless(a::Endpoint{T}, b::Endpoint{Nothing}) where T = true


I can't decide if this makes sense. Especially since we don't define a negative unbounded, and positive unbounded.

fchorney · 2020-05-26T21:47:02Z

src/endpoint.jl

+
+function Base.isless(a::RightEndpoint, b)
+    return !isunbounded(a.endpoint) && (isunbounded(b) || (a.endpoint < b || (a.endpoint == b && !a.included)))
+end


Similar comment here as the one right above.

fchorney · 2020-05-26T21:48:07Z

test/runtests.jl

@@ -8,10 +8,10 @@ using VisualRegressionTests


 @testset "Intervals" begin
-    include("inclusivity.jl")
-    include("endpoint.jl")
+    #include("inclusivity.jl")


I need to remember to add these back in

codecov · 2020-05-26T22:23:45Z

Codecov Report

Merging #92 into master will decrease coverage by 58.55%.
The diff coverage is 87.09%.

@@             Coverage Diff             @@
##           master      #92       +/-   ##
===========================================
- Coverage   96.49%   37.94%   -58.56%     
===========================================
  Files           7        7               
  Lines         257      369      +112     
===========================================
- Hits          248      140      -108     
- Misses          9      229      +220

Impacted Files	Coverage Δ
src/Intervals.jl	`100.00% <ø> (ø)`
src/endpoint.jl	`58.97% <75.00%> (-34.13%)`	⬇️
src/interval.jl	`92.98% <100.00%> (-4.08%)`	⬇️
src/description.jl	`0.00% <0.00%> (-100.00%)`	⬇️
src/anchoredinterval.jl	`0.00% <0.00%> (-94.83%)`	⬇️
src/plotting.jl	`0.00% <0.00%> (-94.12%)`	⬇️
src/inclusivity.jl	`44.44% <0.00%> (-55.56%)`	⬇️

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...7dcac11. Read the comment docs.

nickrobinson251 · 2020-05-27T10:36:06Z

src/endpoint.jl

@@ -1,3 +1,6 @@
+# isnothing doesn't exist in Julia 1.0
+isunbounded(a) = a === nothing


calling this isunbounded rather than isbounded means the code has lots of double-negatives !isunbounded ...i wonder if flipping the condition would make the long chains of && and || a little easier to read

Suggested change

isunbounded(a) = a === nothing

isbounded(a) = a !== nothing

nickrobinson251 · 2020-05-27T10:49:48Z

This PR does a lot of work essentially use nothing as Inf. But we already have Inf.

julia> Interval(-Inf, 100.0)
Interval{Float64}(-Inf, 100.0, Inclusivity(true, true))

julia> Interval(100.0, Inf)
Interval{Float64}(100.0, Inf, Inclusivity(true, true))

Of course, Inf isa Float64, so for Reals it will promote the Intervals to be Interval{Float64}

julia> Interval(π, Inf)
Interval{Float64}(3.141592653589793, Inf, Inclusivity(true, true))

julia> Interval(10//2, Inf)
Interval{Float64}(5.0, Inf, Inclusivity(true, true))

julia> Interval(10, Inf)
Interval{Float64}(5.0, Inf, Inclusivity(true, true))

And for date/time types, there is no promotion

julia> using Dates

julia> Interval(now(), Inf)
ERROR: promotion of types DateTime and Float64 failed to change any arguments

But it does make me wonder if what we want here is just an "InfDateTime"?

omus · 2020-05-27T13:26:35Z

But it does make me wonder if what we want here is just an "InfDateTime"?

We are also looking into such an approach. As intervals only require the provided bounds to be orderable we'd need a representation of infinity for each type wanting to support unbounded endpoints. We're also looking into https://github.com/cjdoris/Infinity.jl but we could take other approaches like you suggest.

I will note however we do not want to promote to Float64 for unbounded integer intervals:

julia> Interval(1, Inf)
Interval{Float64}(1.0, Inf, Inclusivity(true, true))

The Infinity.jl package approach is more suitable:

julia> Interval(1, ∞)
Interval{InfExtended{Int64}}(1, ∞, Inclusivity(true, true))

fchorney · 2020-05-27T14:42:32Z

Yes, I'm thinking maybe at this point in this PR, I should maybe change my efforts to adding a Dates PR to Infinity.jl and just end up using that. There will still be a few changes needed for this package, but I think that would be much easier in the end. This is also assuming that the maintainer of Infinity.jl will be open to a Dates addition.

omus · 2020-06-23T20:31:24Z

Replaced by #104. Note that using Union{T,Nothing} in the struct proposed here results in the type Interval no longer being a bits-type. This was worked around in #104 by always using T and using the type parameters to specify if the lower/upper bound was unbounded.

Add unbounded endpoints. Represented by nothing

139bdaf

omus reviewed May 26, 2020

View reviewed changes

nickrobinson251 reviewed May 26, 2020

View reviewed changes

fchorney added 4 commits May 26, 2020 16:10

Remove Union from Endpoint. Clean up Unbounded logic

b685821

Use 'isunbounded' instead of 'isnothing'

ec04c85

Clean up more boolean operators

fd5fb30

"isnothing" doesn't exist on 1.0

a37d0ae

fchorney commented May 26, 2020

View reviewed changes

Use "isunbounded" in tests

7dcac11

nickrobinson251 reviewed May 27, 2020

View reviewed changes

fchorney mentioned this pull request May 27, 2020

Add support for Infinity.jl #95

Closed

omus closed this Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unbounded endpoints. Represented by `nothing` #92

Add unbounded endpoints. Represented by `nothing` #92

fchorney commented May 25, 2020

omus commented May 26, 2020

omus May 26, 2020

omus May 26, 2020

omus May 26, 2020

omus May 26, 2020

omus May 26, 2020

fchorney May 26, 2020

omus May 26, 2020

omus May 26, 2020

omus May 26, 2020

omus May 26, 2020

fchorney May 26, 2020

omus May 26, 2020

nickrobinson251 May 26, 2020 •

edited

Loading

omus May 26, 2020

fchorney May 26, 2020

omus May 26, 2020

fchorney May 26, 2020

fchorney May 26, 2020

fchorney May 26, 2020

fchorney May 26, 2020

fchorney May 26, 2020

codecov bot commented May 26, 2020 •

edited

Loading

nickrobinson251 May 27, 2020

nickrobinson251 commented May 27, 2020 •

edited

Loading

omus commented May 27, 2020

fchorney commented May 27, 2020

omus commented Jun 23, 2020

	Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) ? true : a < b.endpoint
	Base.isless(a, b::RightEndpoint) = isnothing(b.endpoint) \|\| a < b.endpoint

	Base.isless(a::LeftEndpoint, b) = isnothing(a.endpoint) ? true : a.endpoint < b
	Base.isless(a::LeftEndpoint, b) = isnothing(a.endpoint) \|\| a.endpoint < b

		@@ -1,3 +1,6 @@
		# isnothing doesn't exist in Julia 1.0
		isunbounded(a) = a === nothing

Add unbounded endpoints. Represented by nothing #92

Add unbounded endpoints. Represented by nothing #92

Conversation

fchorney commented May 25, 2020

omus commented May 26, 2020

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

nickrobinson251 May 26, 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

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nickrobinson251 commented May 27, 2020 • edited Loading

omus commented May 27, 2020

fchorney commented May 27, 2020

omus commented Jun 23, 2020

Add unbounded endpoints. Represented by `nothing` #92

Add unbounded endpoints. Represented by `nothing` #92

nickrobinson251 May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

nickrobinson251 commented May 27, 2020 •

edited

Loading