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

Merging with Intervals.jl? #67

Open
rofinn opened this issue Sep 15, 2020 · 12 comments
Open

Merging with Intervals.jl? #67

rofinn opened this issue Sep 15, 2020 · 12 comments

Comments

@rofinn
Copy link

rofinn commented Sep 15, 2020

Both Intervals.jl and IntervalSets.jl define a very similar basic API (e.g., AbstractInterval, inclusivity, set operations). It'd be nice if that basic API could live in one package while many domain specific operations (e.g., math, statistics, time, plotting) lived in others. I like the shorter names in Intervals.jl, but I don't have a strong opinion about where that hierarchy lives. I'd mostly be interested if folks have a view of where the boundary should sit. The merging plan that first comes to mind looks a little like this:

  1. Extend the Intervals.jl hierarchy to match IntervalSets.jl (defining and subtyping Domain)
  2. Strip most of the functionality from Intervals.jl into separate packages (e.g., AnchoredIntervals.jl, IntervalPlots.jl, TimeIntervals.jl)
  3. Ensure that all existing Intervals.jl set related operations and syntax are covered by IntervalSets.jl (e.g., .., intersect, union)
  4. Update IntervalSets.jl to extend the minimal hierarchy in Intervals.jl, but providing the current set operations.

NOTE: This plan requires more work on the Intervals.jl side, and we wouldn't propose touching IntervalSets.jl till the end, but it'd be good to know if step 4 would be reasonable before working on the other components. Feel free to post alternate plans if you disagree.

In the end, IntervalSets.jl would have one extra dependency on the reduced Intervals.jl package and there might need to be some deprecations/backward compatibility where things don't fully align. The benefit to IntervalSets.jl is that features in Intervals.jl would become PnP and the benefit to Intervals.jl is that we'd get to share the same user- and code-base for feedback and development.

@dlfivefifty
Copy link
Member

The whole point of this package was to be the minimal package you describe... so wouldn't it make more sense to let Intervals.jl depend on IntervalSets.jl?

@rofinn
Copy link
Author

rofinn commented Sep 15, 2020

Yes, but based on the names it seems weird for Intervals to be a superset of IntervalSets. Also, I could see folks wanting to work with AbstractIntervals without needing EllipsisNotation.jl? I suppose we could break up and deprecate Intervals.jl all together, but then that seems like a waste of a perfectly good package name?

@timholy
Copy link
Member

timholy commented Sep 15, 2020

IntervalSets = Intervals but only supporting the set operations. I don't personally think the names are something to perseverate on unnecessarily. We can add things to the READMEs to explain as needed.

@omus
Copy link

omus commented Sep 15, 2020

There are a few things that would need to change in IntervalSets.jl to allow the current Intervals.jl to be an extension of it:

  1. Intervals.jl uses singleton bounds types instead of symbols (Open / Closed instead of :open / :closed)
  2. The Interval type in Intervals.jl supports unbounded endpoints. Having this as part of the bounds information has been useful in supporting intervals with element types which do not support infinity (e.g. Int64)
  3. Intervals.jl defines Interval as Interval{T, L <: Bound, R <: Bound} which I think works nicer than Interval{L,R,T} as defined in IntervalSets.jl

Obviously things can also be changed in Intervals.jl but we definitely need to discuss figure out a way forward on these differences.

@dlfivefifty
Copy link
Member

dlfivefifty commented Sep 15, 2020

Very minor differences. Note we also support unbounded endpoints:

julia> d = 1..Inf
1.0..Inf

julia> 0.1 in d
false

julia> 1.1 in d
true

Edit: Sorry I now see what you mean. I don't understand what you mean by Int elements:

julia> d = 1..2
1..2

julia> 1.5 in d
true

@omus
Copy link

omus commented Sep 15, 2020

I don't understand what you mean by Int elements

The interval doesn't require promotion to store an unbounded endpoint:

julia> 1..nothing
Interval{Int64,Closed,Unbounded}(1, nothing)

julia> 1..Inf
Interval{Float64,Closed,Closed}(1.0, Inf)

Also, promotion won't work for non-numeric types:

julia> Date(2020)..nothing
Interval{Date,Closed,Unbounded}(2020-01-01, nothing)

julia> Date(2020)..Inf
ERROR: promotion of types Date and Float64 failed to change any arguments
Stacktrace:
 [1] error(::String, ::String, ::String) at ./error.jl:42
 [2] sametype_error(::Tuple{Date,Float64}) at ./promotion.jl:306
 [3] not_sametype(::Tuple{Date,Float64}, ::Tuple{Date,Float64}) at ./promotion.jl:300
 [4] promote at ./promotion.jl:283 [inlined]
 [5] Interval(::Date, ::Float64) at /Users/omus/.julia/packages/Intervals/T6hJb/src/interval.jl:124
 [6] ..(::Date, ::Float64) at /Users/omus/.julia/packages/Intervals/T6hJb/src/interval.jl:129
 [7] top-level scope at REPL[10]:1

@hyrodium
Copy link
Collaborator

hyrodium commented Sep 28, 2022

  1. Intervals.jl uses singleton bounds types instead of symbols (Open / Closed instead of :open / :closed)
  2. The Interval type in Intervals.jl supports unbounded endpoints. Having this as part of the bounds information has been useful in supporting intervals with element types which do not support infinity (e.g. Int64)
  3. Intervals.jl defines Interval as Interval{T, L <: Bound, R <: Bound} which I think works nicer than Interval{L,R,T} as defined in IntervalSets.jl
  1. What are the practical benefits of singleton bounds types vs symbols? The singleton bounds types seem better on error handling with typos, but is that all of the benefits?
  2. This will be discussed in Add support for unbounded intervals #123.
  3. This difference is similar to SVector{3,Int}, Array{Int,3} vs SVector{Int, 3}, Array{3,Int}. I think the current implementations SVector{3,Int} and Array{Int,3} are well-designed because the type parameters are sorted by importance. I think the type parameters L and R is more important than T, so the current IntervalSets.Interval{L,R,T} seems better.

@rofinn
Copy link
Author

rofinn commented Oct 20, 2022

Sorry, wasn't sure if @omus was planning on answering your comments here.

What are the practical benefits of singleton bounds types vs symbols? The singleton bounds types seem better on error handling with typos, but is that all of the benefits?

It might be misusing the Julia dispatch system, but Intervals.jl often defines specialized methods based on those bounds type parameters. The alternative with symbols would be to push this into a runtime condition or dispatch on something like Val{:open}. I don't think it's a deal breaker, but it is nice to be able to insert a specialized method based on that info.

https://github.com/invenia/Intervals.jl/blob/2999a5da5a589adf126c760c356a0d99f0ddd792/src/interval.jl

This will be discussed in #123.

Cool, thanks for opening that.

I think the type parameters L and R is more important than T, so the current IntervalSets.Interval{L,R,T} seems better

I think this might just be a difference of opinion. Since we work with intervals of time a lot we often want to dispatch on the time type first (ie: UInt32, DateTime, ZonedDateTime) to apply various conversions. Again, I don't think this is a deal breaker, but it would make our conversion method definitions a lot ugly needing to always include the L and R parameters.

@mcabbott
Copy link
Contributor

Could just have an alias for the other order:

julia> RevArray{N,T} = Array{T,N};

julia> [1,2,3] isa RevArray{1}
true

@rofinn
Copy link
Author

rofinn commented Nov 8, 2022

Hmm, that could work, but I don't know how that would work with the symbols for bounds. That being said, @oxinabox raised an important point that parameterizing the interval type by the bounds might not be the best approach anyways. Link to that issue on Intervals.jl is linked above.

@bjarthur
Copy link

bjarthur commented Jan 3, 2024

Intervals.jl now supports sets. does that mean there is no longer a plan to merge with IntervalSets.jl?

FWIW, i chose to go with Intervals.jl as i needed a way to represent and simplify disjoint sets. correct me if i'm wrong, but that is not possible in IntervalSets.jl is it?

@aplavin
Copy link
Contributor

aplavin commented Jan 3, 2024

I assume by "disjoint sets" you mean unions of intervals, and not something like https://en.wikipedia.org/wiki/Disjoint-set_data_structure? Then yes, IntervalSets itself doesn't support it: this package is focused on intervals themselves.
In isolation it may not matter much which package to use, but is you want interop with intervals in the wider Julia ecosystem then IntervalSets is the clear choice: it's much more popular. As a bonus, it's also dependency-free.

Some time ago, I made a small package IntervalUnions.jl that supports unions of intervals on top of IntervalSets.jl. Maybe it already supports the features you need? For now IntervalUnions.jl is quite barebones, but works fine.

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

No branches or pull requests

8 participants