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

Fix union and union! of intervals #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix union and union! of intervals #74

wants to merge 1 commit into from

Conversation

iamed2
Copy link
Member

@iamed2 iamed2 commented Jun 14, 2019

For the purposes of these definitions, a "set" of intervals is either a single AbstractInterval or a vector of AbstractIntervals.

This now supports the methods that set union supports. Notably, the method @morris25 found will now throw an ArgumentError rather than falling back to the Base definition and giving a different result.

Fixes #73

Before:

julia> target
10-element Array{AnchoredInterval{-1 hour,ZonedDateTime},1}:
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 14, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 14, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 8, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 8, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 8, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 8, tz"UTC"), Inclusivity(false, true))

julia> a = union(target)
3-element Array{AbstractInterval,1}:
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 12, 6, tz"UTC"), ZonedDateTime(2013, 2, 12, 8, tz"UTC"), Inclusivity(false, true))
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 13, 6, tz"UTC"), ZonedDateTime(2013, 2, 13, 8, tz"UTC"), Inclusivity(false, true))
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 14, 6, tz"UTC"), ZonedDateTime(2013, 2, 14, 7, tz"UTC"), Inclusivity(false, true))

julia> b = union!(target)
5-element Array{AnchoredInterval{-1 hour,ZonedDateTime},1}:
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 14, 7, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 12, 8, tz"UTC"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2013, 2, 13, 8, tz"UTC"), Inclusivity(false, true))

After:

julia> a = union(target)
3-element Array{AbstractInterval,1}:
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 12, 6, tz"UTC"), ZonedDateTime(2013, 2, 12, 8, tz"UTC"), Inclusivity(false, true))
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 13, 6, tz"UTC"), ZonedDateTime(2013, 2, 13, 8, tz"UTC"), Inclusivity(false, true))
 Interval{ZonedDateTime}(ZonedDateTime(2013, 2, 14, 6, tz"UTC"), ZonedDateTime(2013, 2, 14, 7, tz"UTC"), Inclusivity(false, true))

julia> b = union!(target)
ERROR: ArgumentError: Cannot `union!` array of intervals of type AnchoredInterval{-1 hour,ZonedDateTime} as the type may change
Stacktrace:
 [1] union!(::Array{AnchoredInterval{-1 hour,ZonedDateTime},1}) at /Users/ericdavies/.julia/dev/Intervals/src/interval.jl:295
 [2] top-level scope at none:0

@iamed2 iamed2 requested a review from omus as a code owner June 14, 2019 17:07
src/interval.jl Show resolved Hide resolved
throw(ArgumentError(
"Cannot `union!` array of intervals of type $T as the type may change"
))
end
Copy link
Collaborator

@omus omus Jun 14, 2019

Choose a reason for hiding this comment

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

It would be better to use dispatch here for future interval types can support their own union! methods

function Base.union!(intervals::AbstractVector{<:AbstractInterval})
    throw(ArgumentError("Cannot `union!` array of intervals of type $(eltype(intervals)) as the type may change"))
end

Base.union!(intervals::AbstractVector{<:Union{AbstractInterval,Interval}) = ...

(I think this works)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't the existing definition already allow that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think my point here was that this approach in the PR isn't the best for new interval types that want to define their own union! method.

@omus
Copy link
Collaborator

omus commented Jun 14, 2019

Fixes: #73

src/interval.jl Show resolved Hide resolved
function Base.union(intervals::AbstractVector{<:AbstractInterval})
return union!(convert(Vector{AbstractInterval}, intervals))
function Base.union(
interval::Union{AbstractInterval, AbstractVector{<:AbstractInterval}}, intervals...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do:

Suggested change
interval::Union{AbstractInterval, AbstractVector{<:AbstractInterval}}, intervals...,
interval::Union{AbstractInterval, AbstractVector{<:AbstractInterval}}, intervals::Union{AbstractInterval, AbstractVector{<:AbstractInterval}}...,

Avoids a call such as union(Interval(1,2), 3) causing a confusing error

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.

Intervals.union and Intervals.union! don’t give the same output
2 participants