-
Notifications
You must be signed in to change notification settings - Fork 27
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
1..2 should expand to Interval(1.0, 2.0); rand(Interval(1,2)) should return an Int #117
Comments
I still have the same thoughts in this comment (#115 (comment)), and I think the proposal is undesirable.
I think the docstring is misleading and should be fixed. The type parameter
How about
Does this imply I think what we have to do are:
|
The behavior of julia> 1.0 in [1,2,3]
true
julia> 1.0 isa eltype([1,2,3])
false So, I think the correct meaning of julia> 1 in Float32[1,2,3]
true
julia> 1.0 in Float32[1,2,3]
true
julia> nextfloat(1.0) in Float32[1,2,3]
false
julia> Float32(nextfloat(1.0)) in Float32[1,2,3]
true In this concept, julia> a = nextfloat(BigFloat(1.2)) # cannot converted to Float64 loss-less
1.199999999999999955591079014993738383054733276367187500000000000000000000000017
julia> Float64(a) == a
false
julia> a in 1.0 .. 2.0
true I think the most correct definition of |
I disagree with this completely. I'm still of the opinion that integer endpoints should remain integers. In particular, we are using I think the real issue is the use of |
Which type should |
I think if |
Okay. That sort of implies that we settle on a I don't mind the outcome of this discussion, but by now I do feel strongly about having the |
I would actually say the opposite: |
If there is a function that returns the
I think the pull request aimed to improve the user experience, without changing the fundamental issues. Of course some of the problems would go away if this package did not define I think we are free to define ourselves what For this package, I'd say |
Also, to add to @hyrodium's convincing list of examples above, the eltype of an interval is not julia> 2.0+0im ∈ 1..2
true |
Yes exactly. What I mean is that julia> Real(1+0im) == 1+0im
true |
Yes, and I agree that by that definition it is impossible to correctly define I somewhat like the suggestion above to define It might work for intervals and I'm fine with that - we'll just adapt in the DomainSets package. |
Personally I think IntervalSets.jl and DomainSets.jl should be rewritten so that a domain is an interface not an abstract type. That is, a domain is anything that implements Where is It's used in ApproxFun.jl but that can be easily changed by implementing |
I checked last time the discussion came up, and IntervalSets doesn't use |
Currently, Julia does not lend itself very well to that idea in my experience. There is a hidden trade-off. If you can't say something is a domain based on its type, then any indication that you want to treat arguments to a function as a domain has to be in the function name. That is why DomainSets already has I think this is why so many package ecosystems have a few core abstract types, including famous ones like |
Actually, it seems like the best solution might be
Then Here are my responses to some of the more important questions above:
As I mentioned in the other thread,
@dlfivefifty I feel the same way, though I am considering @daanhb's arguments so I have not formulated an opinion. @daanhb if there is more literature to read on this, I'd certainly be interested.
Keep in mind that there is no |
The reasoning rapidly leads to formalizing interfaces. I'm not sure what the literature is on this topic. It seems likely that the trade-off I mentioned is a known thing though. Anyway, this goal suggests the exercise of formulating an interface for Domains, like the one for iterable objects, and see what kind of functionality can be made from that. But let's not do that here. |
abstract type AbstractInterval{T, E} <: Domain{T} end I think this is the most sensible proposal so far. We should come up with a well-defined definition of |
How can we define the type parameter
Yes, that's not ideal. The IntervalSets.jl/src/interval.jl Line 225 in 41028c0
|
That's the wrong way round. for |
Since in this scenario I agree that adding type parameters is not ideal, especially if they are pervasive, i.e., all subtypes have them. |
Note that DomainSets.jl seems to use the type parameter mostly for conversion. E.g. Why this needs to be done needs to be thought through if we want to do a major redesign. |
Warning: long post. The thing that stings about this solution is that there are too many type parameters. This is a typical problem: generality leads to more parameters, but the common case is simple. In the current setting, in most cases There is a way to reconcile both, but it requires a bit more programming and, as always, just one more layer of abstraction. The design pattern goes as follows. You make an abstract type with two type parameters. Here, that might be abstract type AbstractInterval{T,E} end All functionality of intervals is implemented for this type, in terms of eltype(::Type{<:AbstractInterval{T,E}}) where {T,E} = T
boundstype(d::AbstractInterval) = boundstype(typeof(d))
boundstype(::Type{<:AbstractInterval{T,E}}) where {T,E} = E The functionality for intervals follows, always for Then, at the end, you do: struct SimpleInterval{T} <: AbstractInterval{T,T}
a :: T
b :: T
end
leftendpoint(d::SimpleInterval) = d.a
rightendpoint(d::SimpleInterval) = d.b
struct ComplicatedInterval{T,E} <: AbstractInterval{T,E}
a :: E
b :: E
end
leftendpoint(d::ComplicatedInterval) = d.a
rightendpoint(d::ComplicatedInterval) = d.b Both concrete intervals "just work" with all functionality of intervals. A simple interval is one where the The Then, somebody else in some other package could do: struct MySpecialIntegerInterval <: AbstractInterval{Int,Int}
a :: Int
end
leftendpoint(d::MySpecialInterval) = d.a
rightendpoint(d::MySpecialInterval) = 5 This is some interval with an integer left point and A bonus is to define the abstract constructor AbstractInterval(a::Int, b::Int) = ComplicatedInterval{float(Int),Int}(a, b)
AbstractInterval(a::T, b::T) where {T <: AbstractFloat} = SimpleInterval{T}(a,b) The contract is that this constructor always return one of its concrete subtypes. It just depends on the types of the arguments which one you'll get. But it will be optimized for your use case. As a user, you won't need to know what the name of the best fitting type is (and it would be bad practice to write code that depends on the specific concrete subtype). In this case I'd rather call the abstract type Finally, the Both IntervalSets and DomainSets already use this design pattern to some extent. The aspect I want to emphasise here is that you can use it to simplify the types you'll most often be dealing with in the common cases, while still catering to the specialist out there, i.e., you're not giving up on generality. |
Here is a working example of what I mean, over in DomainSets. An Scalars: julia> using DomainSets
julia> m = AffineMap(2.0, 3)
x -> 2.0 * x + 3.0
julia> typeof(ans)
DomainSets.ScalarAffineMap{Float64} Arrays and vectors: julia> AffineMap(rand(2,2), rand(2))
x -> A * x + v
A = [0.6369548300001839 0.6384788626918244; 0.8296441404786224 0.16026239514708762]
v = [0.3984510604775833, 0.2192951933222833]
julia> typeof(ans)
DomainSets.VectorAffineMap{Float64} Something of the form julia> m = AffineMap(2, rand(2,1))
x -> 2.0 * x + A
A = [0.527326545615108; 0.03894785911249521;;]
julia> m([1,2])
2×1 Matrix{Float64}:
2.527326545615108
4.038947859112495
julia> typeof(m)
DomainSets.GenericAffineMap{Matrix{Float64}, Float64, Matrix{Float64}} |
Ok, I think the general approach of having the bounds type be (possibly) different from the eltype seems to be the best candidate solution. I will think about the details and propose something in a new issue. The issue of what does |
Based on the discussions in #115, I am going to submit a pull request with the following changes:
1..2
will expand toInterval(1.0, 2.0)
(actually anya::Integer..b::Integer
will be converted toInterval(float(a), float(b))
).rand(Interval(1, 2))
will return anInt
The justification for change 1 goes like this:
1..2
, they are imagining a set of real numbers between 1 and 2.1..2
isInterval(1.0, 2.0)
. This is similar to the way1:0.01:2
expands to1.0:0.01:2.0
.Interval(1, 2)
, they can still do it explicitly.The justification for change 2 goes like this:
Domain{T}
says that it is a "subset of typeT
"rand
on a set should return an element of that set, hencerand(Interval(1, 2))
should return anInt
.Maintainers, what do you think about this?
The text was updated successfully, but these errors were encountered: