Deprecate methods for eltype and add boundstype

[hyrodium]

Closes #115 and #122.

We had a long discussion about eltype, and I believe removing them is the best. (#115 (comment))

• eltype should not be defined for non-iterable object
• We can keep maintaining the implementation as a different function.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (467f76e) 99.17% compared to head (60fa9e8) 99.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   99.17%   99.18%   +0.01%     
==========================================
  Files           4        4              
  Lines         242      245       +3     
==========================================
+ Hits          240      243       +3     
  Misses          2        2              

Files Changed	Coverage Δ
src/IntervalSets.jl	98.37% <100.00%> (+0.04%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zsunberg]

Does this mean that the endpoints will always need to be the same type? Might someone want to have a 1.3 to pi interval where they do not want pi converted to Float64?

[aplavin]

Sorry, but want to raise the same point again :) Please consider the option to simply deprecate eltype, so that not to make a breaking release over a trivial change. It's extremely easy in this case, just use @deprecate.

[hyrodium]

Does this mean that the endpoints will always need to be the same type?

Yes, at least for now.

Might someone want to have a 1.3 to pi interval where they do not want pi converted to Float64?

Yeah, I thought we should not separate the types of endpoints when I created this PR, but this should be done in another PR if someone wants that feature. (I personally would like to avoid that feature, but this should be discussed in other issues or PRs.)

Please consider the option to simply deprecate eltype, so that not to make a breaking release over a trivial change. It's extremely easy in this case, just use @deprecate.

Yes! I'm planning to release the next v0.7.x version with @deprecate in v0.7.x branch, and release the next breaking release from the master branch. I just want to get some reviews to boundstype in this PR.

EDIT: Sorry, I got screwed up, I will update this PR and release the next v0.7.8 from master branch.

[hyrodium]

I will merge this PR in a few days if there are no objections.

[dlfivefifty]

so that not to make a breaking release over a trivial change

Warnings basically break all downstream packages, so this should be treated as a breaking change. Leaving in the deprecation for the next version is fine as it helps debug.

[dlfivefifty]

@daanhb This will have a big impact on DomainSets.jl. Speak now or forever hold your peace.

[aplavin]

Warnings basically break all downstream packages, so this should be treated as a breaking change.

Isn't the whole purpose of deprecation warnings to be non-breaking? Lots of packages use depwarns exactly for this.
They are disabled by default in Julia and won't have any effect aside from tests.

[zsunberg]

Warnings basically break all downstream packages, so this should be treated as a breaking change. Leaving in the deprecation for the next version is fine as it helps debug.

@dlfivefifty I am curious what you mean by this. Deprecation warnings are turned off by default except in tests, so deprecating should be treated as a warning that something will happen in the future, not that it is already breaking. https://stackoverflow.com/questions/56135768/which-part-of-semver-should-i-bump-when-deprecating-supported-python-version

(Edit: Ah, I see that @aplavin already responded to this)

[daanhb]

@daanhb This will have a big impact on DomainSets.jl. Speak now or forever hold your peace.

I don't like deadlines on vacation :-)

If this PR does have a big impact on DomainSets.jl, then I hope it does not happen this way. I think everybody agreed that we should move the definition of Domain{T} to a new package such as DomainSetsCore.jl, a very related topic, and I strongly feel that should happen first. I'd be happy to propose something in a few weeks.

I'm not sure this change will be very breaking for now, though one has to check. As things stand, DomainSets.jl defines the eltype of any domain anyway, including intervals.

For the record, I do not intend to change that. If something truly incompatible happens in IntervalSets, I'm more inclined to diverge away, as for the purposes of computations (perhaps less relevant in JuliaMath than in JuliaApproximation) having an eltype to me is clearly way better and simpler than any alternatives - as I said before for me that ship has sailed.

[hyrodium]

If this PR does have a big impact on DomainSets.jl, then I hope it does not happen this way. I think everybody agreed that we should move the definition of Domain{T} to a new package such as DomainSetsCore.jl, a very related topic, and I strongly feel that should happen first. I'd be happy to propose something in a few weeks.

I agree with having DomainSetsCore.jl.
I thought we may have AbstractInterval{S} <: DomainSetsCore.Domain (S is boundstype) without the type parameter of Domain, but is this less informative? If we need more information, is it better to have Domain{T, N} which represents the typical element type T and the dimension of the domain N? (For example, a domain on a sphere may be <: Domain{SVector{3,Float64}, 2})

I don't have practical usage of Domain with type parameters for now, so some examples would be preferred. (I will look into DomainSets.jl)

having an eltype to me is clearly way better and simpler than any alternatives

Is it okay to rename eltype to typical_eltype just for avoiding confusion such as #115? (The name typical_eltype is temporary, just for discussion.)

[daanhb]

I agree with having DomainSetsCore.jl. I thought we may have AbstractInterval{S} <: DomainSetsCore.Domain (S is boundstype) without the type parameter of Domain, but is this less informative? If we need more information, is it better to have Domain{T, N} which represents the typical element type T and the dimension of the domain N? (For example, a domain on a sphere may be <: Domain{SVector{3,Float64}, 2})

The combination {T,N} is less general and flexible than having T = SVector{N,S}, because the former is exclusive to Euclidean geometry and the latter isn't - so I think one parameter T will do. As one example that is relevant in practice, T = Vector{S} allows domains with an arbitrary ("flexible") dimension not encoded in the type, but {T,N} doesn't.

This argument only holds for the abstract supertype. A concrete domain could always do MyEuclideanDomain{T,N} <: Domain{SVector{T,S}}. Or MyDomain <: Domain{Any}, if eltype is not relevant.

I don't have practical usage of Domain with type parameters for now, so some examples would be preferred. (I will look into DomainSets.jl)

The simplest examples arise from taking cartesian products:

julia> using IntervalSets, DomainSets

julia> using DomainSets: ×

julia> d1 = (1..2) × (3..4.0)
(1.0 .. 2.0) × (3.0 .. 4.0)

julia> eltype(d1)
SVector{2, Float64} (alias for StaticArraysCore.SArray{Tuple{2}, Float64, 1, 2})

julia> d2 = (1.0..2.0) × UnitCircle()
(1.0 .. 2.0) × UnitCircle()

julia> eltype(d2)
SVector{3, Float64} (alias for StaticArraysCore.SArray{Tuple{3}, Float64, 1, 3})

julia> d3 = (1.0..2.0) × UnitBall(5)
(1.0 .. 2.0) × UnitBall(5)

julia> eltype(d3)
Tuple{Float64, Vector{Float64}}

There is a lot going on here, but these are the sort of things on my mind when I say that T conveys "structure" of the domain.

Is it okay to rename eltype to typical_eltype just for avoiding confusion such as #115? (The name typical_eltype is temporary, just for discussion.)

I feel like typical_eltype is more confusing than eltype. My guess is that having a single intended eltype, for those who care about element types, is the most common case. (There might be other element types, sure, but at least this one works.)

[daanhb]

I took a stab at DomainSetsCore.jl.

The first iteration has AbstractDomain and Domain{T} <: AbstractDomain. IntervalSets could do one of the following:

• let AbstractInterval{T} inherit from Domain{T}: in that case T is the eltype
• let AbstractInterval or AbstractInterval{T} inherit from AbstractDomain: in that case the eltype is Any (the default eltype of anything)
• ignore DomainSetsCore.jl and let AbstractInterval be the main supertype of this package

In the long run I'd say the third option may be the preferred one. It is also closest to what @dlfivefifty has been pushing for, namely domains as an interface. Ideally, we could get DomainSets.jl to work with Intervals like that, regardless of inheritance of any supertype. Because that would mean that DomainSets could also work with other packages defining domains, with all packages able to evolve independently.

To me, in this design AbstractDomain is functionally equivalent to Domain{Any} and I think I'd rather remove it.

I've added an AsDomain constructor for experimentation. The idea is that one can do some_function(a, b, AsDomain(c)) to hint that c behaves like and should be interpreted as a domain, much like Ref(x) is used in broadcast to hint at something. One might then be able to do 1..2 \cup AsDomain([4,5,6]) and get the union of the interval with the vector, without actually ever converting the vector to a wrapper Domain type.

[zsunberg]

ignore DomainSetsCore.jl and let AbstractInterval be the main supertype of this package

@daanhb , I was about to log on and suggest this! I think DomainSets would be more powerful if it relies as little as possible on things being a subtype of Domain.

[daanhb]

There was already an issue for DomainSetsCore.jl with some discussion: #136. I'll move there.

[daanhb]

Coming back to the issue at hand: so far I've sticked to Domain{T} as a supertype, because I don't know how to build DomainSets otherwise. The T is quite pervasive. But perhaps a function like domaineltype is indeed better for the "domain interface" in the long run, since it doesn't clash with anything. The default could still be domaineltype(d) = eltype(d), but crucially domaineltype would be "owned" by DomainSetsCore.

Conceptually, its meaning comes from answering the question: if you were to sample the domain, what would the type of a point be? (Currently, rand(1..2) returns a Float64.) That type is really associated with any kind of discretization of the continuous set, which is eventually what we're after in JuliaApproximation.

I was looking at other packages, such as GeometryBasics. There you have:

julia> rect = Rect(Vec(0.0, 0.0), Vec(1.0, 1.0))
Rect2{Float64}([0.0, 0.0], [1.0, 1.0])

julia> eltype(ans)
Any

In order to combine those rectangles with other domains, one would have to define eltype for them, which risks changing how they behave elsewhere. That seems a no-no.

So I guess I'm changing my mind here. I don't know what best to do yet with intervals inheriting from Domain{T} though.

[daanhb]

Meanwhile, a lot of changes happened over in DomainSets (unmerged, but in this PR), which are beneficial independently. Three possibilities to move forward are:

• we incorporate the changes in DomainSets itself, release v0.7, and IntervalSets.jl does not change
• we move the definition of Domain{T} from IntervalSets.jl to DomainSetsCore.jl and make two new releases. In this case the functionality of IntervalSets.jl remains the same
• the intervals in IntervalSets no longer inherit from a Domain supertype and just adhere to the domain interface. (This will probably require further changes in DomainSets to ensure that things remain easy to use)

I think that, if there is a common Domain supertype, it should be in a separate package. I think that it is not worth pursuing convincing types from other packages to inherit from Domain: making the interface easier to work with is clearly the way to go. I've found that few other domain-like types implement eltype the way DomainSets expects it, so working with domaineltype (name still up for discussion, preferably something shorter) also seems to way to go.

Based on this, for now I prefer the second option above: we register DomainSetsCore, move the definition of Domain{T} there, and make no other changes. That allows to experiment with domains as an interface, hopefully fix and improve things, and then move on.

As for IntervalSets, I believe the goal should be that the simplest possible sensible definition of an interval, namely

struct Interval{T}
    a::T
    b::T
end

should "just work". That won't work yet with option 2 above. For anyone wishing to do computations with such intervals as a continuous set, a good question to ask is: how does one (i) discover and (ii) control the type being returned by rand(d) for an interval in which the endpoints are integers.

[zsunberg]

how does one (i) discover and (ii) control the type being returned by rand(d) for an interval in which the endpoints are integers.

Regarding discovery, there is Random.gentype. However, it is not documented, so maybe it should not be used outside of Base?

I think that if we adopt the struct proposed above, we should just convert every sampled type to the T anyways, i.e. rand(1..2) should return an Int. It might be a bit of a pain to deal with something like the open interval (1,2), but that does not seem like a show stopper. People will quickly start using 1.0..2.0 instead of 1..2.

For controlling the type returned, one option would be

struct Interval{T, A, B}
    a::A
    b::B
end

This has the added benefit of allowing different endpoint types, which could be useful for, e.g. 1..pi.

[hyrodium]

Sorry for the late response.
Let me summarise the current discussions.

• Base.eltype
  • Typically, this function is used for iterators.
  • We all agree with that eltype(1..2) == Int is problematic, right?
  • I'm not sure eltype(1..2) == Float is the way to go. (Should eltype(1..2) be Float64? (currently it is Int) #115 (comment))
• Random.gentype
  • This is not well-documented and may be removed in the future. (the future of Random.gentype JuliaLang/julia#31968 (comment))
  • But I think this function should be stable and documented; eltype for iterators, gentype for random generators.
  • This package defines gentype(::Type{<:Interval}) because I would like to keep the behavior of eltype(::Type{<:Interval}) (Add support for rand(1..2) #104).

  • IntervalSets.jl/src/interval.jl

    Line 162 in 6545e5d

    Random.gentype(::Type{Interval{L,R,T}}) where {L,R,T} = float(T)

• IntervalSets.boundstype
  • A useful function to obtain the type of the boundaries of an interval.
• DomainSetsCore.domaineltype
  • documentation
  • We don't have agreements on the proper definition of "eltype", so let's define our function!

I think the following definitions would be acceptable.

Base.eltype(::Type{<:Interval})  # This is not defined
Random.gentype(::Type{Interval{L,R,T}}) where {L,R,T} = float(T)
Base.boundstype(::Type{Interval{L,R,T}}) where {L,R,T} = T
DomainSetsCore.domaineltype(::Type{Interval{L,R,T}} = float(T)  # I don't have usecases of this method currently, but this would be useful in DomainSets.jl, right?