Unbounded interval support #104

omus · 2020-06-10T18:15:03Z

Using the parametric bounds interface in #102 this PR adds the fundamentals for constructing and interacting with unbounded intervals.

Intervals with unbounded endpoints can be constructed by using nothing as a placeholder (e.g. Interval(nothing, 2) creates the interval (,2]). Explicitly stating the bound type requires that when supplying nothing that the respectively stated bound type is Unbounded (e.g. Interval{Unbounded,Open}(nothing, 3) creates (,3) but Interval{Open,Open}(nothing, 3) and Interval{Unbounded,Unbounded}(nothing, 3) both raise MethodErrors).

As for the actual implementation for Interval{T} where T is the element type we always store values of type T and never store Nothing. Doing this avoids using a union type which would result in Interval no longer being a bits type. However, using the first or last function on an unbounded endpoint does now return nothing.

The current implementation has span throw an exception on unbounded intervals as discussed in #89 (comment).

codecov · 2020-06-10T18:18:28Z

Codecov Report

Merging #104 into master will increase coverage by 2.34%.
The diff coverage is 94.02%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   69.94%   72.28%   +2.34%     
==========================================
  Files           8        8              
  Lines         376      415      +39     
==========================================
+ Hits          263      300      +37     
- Misses        113      115       +2

Impacted Files	Coverage Δ
src/Intervals.jl	`100.00% <ø> (ø)`
src/plotting.jl	`94.11% <0.00%> (ø)`
src/interval.jl	`96.00% <93.54%> (-1.20%)`	⬇️
src/endpoint.jl	`94.73% <95.00%> (+1.18%)`	⬆️
src/anchoredinterval.jl	`98.57% <100.00%> (+0.35%)`	⬆️

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 78b125a...b055421. Read the comment docs.

src/anchoredinterval.jl

omus · 2020-06-10T18:45:14Z

src/anchoredinterval.jl

@@ -246,6 +266,8 @@ function Base.isempty(interval::AnchoredInterval{P,T}) where {P,T}
    return P == zero(P) && !isclosed(interval)
 end

+# TODO: Not required but should be addressed


Optimized intersect for AnchoredIntervals. This function may need to be dropped as it it may no longer possible to return an AnchoredInterval for unbounded results here. I need to think about this more.

src/endpoint.jl

omus · 2020-06-12T12:40:34Z

Rebased now that #102 has been merged.

src/anchoredinterval.jl

src/Intervals.jl

src/anchoredinterval.jl

src/endpoint.jl

iamed2 · 2020-06-12T18:23:38Z

src/anchoredinterval.jl

@@ -60,6 +60,17 @@ See also: [`Interval`](@ref), [`HE`](@ref), [`HB`](@ref)
 """
 struct AnchoredInterval{P, T, L <: Bound, R <: Bound} <: AbstractInterval{T,L,R}
    anchor::T
+
+    function AnchoredInterval{P,T,L,R}(anchor::T) where {P, T, L <: Bound, R <: Bound}
+        if P < zero(P) && R === Unbounded || P > zero(P) && L === Unbounded


Should we really include support for unbounded AnchoredIntervals? Do you expect people to pass infinite periods? I think it makes sense to restrict AnchoredIntervals to finite P.

A very valid question. Just because we can support it doesn't mean we should. I'll give that some thought

One argument for keeping this is that removing Unbounded support doesn't stop people from using infinite periods. We'd have to add checks to disable that as well

Thinking about this more I think support for unbounded and infinity should be consistent. If we do want to remove unbounded support entirely from AnchoredInterval we should also remove support for infinity.

My rational in why these go together is that I was planning on defining equality between infinite and unbounded intervals such that [∞,x] == (,x]. Not having unbounded support but having infinity support seems wrong to me considering this.

Thinking about this more I think support for unbounded and infinity should be consistent.

Yeah I do agree.

Okay, I can imagine wanting this rather than just using Interval I guess. But it's hard to imagine why you'd want an AnchoredInterval with infinity as part of the type.

Okay, I'm okay with keeping this functionality. But a MethodError hurts readability for the error, since the user cannot see from the type signature why this is wrong. I think this should be a different type of error.

One way to keep it a MethodError would be to add yet another type parameter that's just the sign of the period, then all the relevant information would be available for dispatch.

I'll make it an argument error. Not entirely accurate but better

More thoughts on this: #104 (comment)

src/anchoredinterval.jl

omus · 2020-06-12T21:05:40Z

In reviewing the code again and taking this conversation into consideration. I've noticed that there are some cases with AnchoredIntervalwhere non-finite values are allowed as the value for the anchor endpoint. The reason this is problematic is for the same reason we don't allow the anchor endpoint to be unbounded: it makes it impossible to calculate the other endpoint.

I've unfortunately run out of time to work on this and ensure the problem is solved correctly so I'll be holding off on merging this PR. I do believe the unbounded support for the Interval type is fully functional at this time.

#104 (comment)

omus · 2020-06-23T16:43:06Z

src/anchoredinterval.jl

+end
+
+_isfinite(x) = iszero(x - x)
+_isfinite(x::Real) = Base.isfinite(x)


I'm attempting to get this functionality added to base: JuliaLang/julia#36380

src/docstrings.jl

nicoleepp · 2020-06-23T17:37:05Z

src/plotting.jl

@@ -3,7 +3,8 @@ interval_markers(L::Type{<:Bound}, R::Type{<:Bound}) = interval_marker.([L, R])
 interval_marker(x::Type{Closed}) = :vline
 interval_marker(x::Type{Open}) = :none

-@recipe function f(xs::AbstractVector{<:AbstractInterval{T}}, ys) where T
+# TODO: Add support for plotting unbounded intervals


Open an issue and add to this comment?

Yes, thanks. I did look into this and it got complicated quickly

nicoleepp

LGTM

The current implementation does not correctly support intervals with unbounded endpoints and from some basic attempts it will probably be an involved change.

Ensure that we can construct an AnchoredInterval where the computed endpoint is an non-existent or ambiguous ZonedDateTime.

#104 (comment)

omus · 2020-06-23T18:32:08Z

Rebased to apply documentation fixups and fixed typo in commit message. Just waiting on CI then will merge.

omus changed the title ~~Core unbounded interval support~~ WIP: Core unbounded interval support Jun 10, 2020

omus commented Jun 10, 2020

View reviewed changes

src/anchoredinterval.jl Outdated Show resolved Hide resolved

omus commented Jun 10, 2020

View reviewed changes

omus force-pushed the cv/parametric-bounds branch 2 times, most recently from 88f26f6 to 70443cb Compare June 11, 2020 18:53

omus force-pushed the cv/unbounded-parametric branch from 81c4663 to 1f9e74a Compare June 11, 2020 19:02

fchorney reviewed Jun 11, 2020

View reviewed changes

src/endpoint.jl Show resolved Hide resolved

omus force-pushed the cv/parametric-bounds branch 2 times, most recently from a59deb6 to f24a406 Compare June 12, 2020 05:16

Base automatically changed from cv/parametric-bounds to master June 12, 2020 12:33

omus added 9 commits June 12, 2020 07:33

Core support for unbounded Endpoint

7cc0127

Core support for unbounded Interval

dc7893f

Core support for unbounded AnchoredInterval

8417a4e

Throw exception when calculating unbounded span

2778d04

Unbounded endpoints should ignore value in hash

8f7b1b6

Unbounded Endpoint constructors using nothing

c98abe6

Unbounded Interval constructors using nothing

588055a

Update AnchoredInterval to use nothing constructors

212604a

first/last return nothing for unbounded endpoints

1220a2c

omus force-pushed the cv/unbounded-parametric branch from 1f9e74a to 1220a2c Compare June 12, 2020 12:34

omus changed the title ~~WIP: Core unbounded interval support~~ Unbounded interval support Jun 12, 2020

omus commented Jun 12, 2020

View reviewed changes

src/anchoredinterval.jl Outdated Show resolved Hide resolved

nicoleepp reviewed Jun 12, 2020

View reviewed changes

src/Intervals.jl Show resolved Hide resolved

nickrobinson251 reviewed Jun 12, 2020

View reviewed changes

src/anchoredinterval.jl Outdated Show resolved Hide resolved

nickrobinson251 reviewed Jun 12, 2020

View reviewed changes

src/endpoint.jl Outdated Show resolved Hide resolved

iamed2 reviewed Jun 12, 2020

View reviewed changes

nicoleepp mentioned this pull request Jun 15, 2020

Support unbounded ranges JuliaDatabases/LibPQ.jl#184

Merged

omus added a commit that referenced this pull request Jun 23, 2020

Disallow support for non-bounded AnchoredInterval

d6cf602

#104 (comment)

omus force-pushed the cv/unbounded-parametric branch from 09c3fef to d6cf602 Compare June 23, 2020 16:28

omus mentioned this pull request Jun 23, 2020

Deprecate convert(AnchoredInterval{Ending}, ...) and convert(AnchoredInterval{Beginning}, ...) #109

Open

omus commented Jun 23, 2020

View reviewed changes

TylerLoewen reviewed Jun 23, 2020

View reviewed changes

src/docstrings.jl Outdated Show resolved Hide resolved

src/docstrings.jl Outdated Show resolved Hide resolved

nicoleepp reviewed Jun 23, 2020

View reviewed changes

This was referenced Jun 23, 2020

Add plotting support for unbounded intervals #110

Open

Deprecate reordering of Interval endpoints #97

Merged

nicoleepp approved these changes Jun 23, 2020

View reviewed changes

omus added 15 commits June 23, 2020 13:30

Add docstrings for accessors

ef026d5

Use DomainError in span

05155a3

Rename internal span fallback function

c5c3adb

Disable plotting support for non-bounded intervals

11665cf

The current implementation does not correctly support intervals with unbounded endpoints and from some basic attempts it will probably be an involved change.

Documentation for bound types

d914ea0

Re-enable custom intersect for AnchoredIntervals

0166211

Refactor endpoint hashing

f8ddd0b

Use dispatch to skip invalid comparison tests

5669457

Endpoint conversion constructor

896022b

Improve AnchoredInterval tests

663fb0b

Throw ArgumentError for constructing invalid AnchoredInterval

5dd0ffc

Define rules for valid AnchoredInterval

b85f582

Test AnchoredInterval with invalid computed bound

3fdeb3c

Ensure that we can construct an AnchoredInterval where the computed endpoint is an non-existent or ambiguous ZonedDateTime.

Disallow unbounded anchors

c0f946a

Disallow support for non-bounded AnchoredInterval

b055421

#104 (comment)

omus force-pushed the cv/unbounded-parametric branch from 61f8560 to b055421 Compare June 23, 2020 18:31

omus merged commit 153ce03 into master Jun 23, 2020

omus deleted the cv/unbounded-parametric branch June 23, 2020 19:39

omus mentioned this pull request Jun 23, 2020

Add unbounded endpoints. Represented by nothing #92

Closed

omus mentioned this pull request Aug 14, 2020

Add support for Infinity.jl #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unbounded interval support #104

Unbounded interval support #104

omus commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 10, 2020 •

edited

Loading

omus Jun 10, 2020

omus commented Jun 12, 2020

iamed2 Jun 12, 2020

omus Jun 12, 2020

omus Jun 12, 2020

omus Jun 12, 2020

iamed2 Jun 12, 2020

iamed2 Jun 12, 2020

omus Jun 12, 2020

omus Jun 23, 2020

omus commented Jun 12, 2020

omus Jun 23, 2020

nicoleepp Jun 23, 2020

omus Jun 23, 2020

omus Jun 23, 2020

nicoleepp left a comment

omus commented Jun 23, 2020

Unbounded interval support #104

Unbounded interval support #104

Conversation

omus commented Jun 10, 2020 • edited Loading

codecov bot commented Jun 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

omus commented Jun 12, 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

omus commented Jun 12, 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

nicoleepp left a comment

Choose a reason for hiding this comment

omus commented Jun 23, 2020

omus commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 10, 2020 •

edited

Loading