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

Additional accessor and convert functions #108

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sprockmonty
Copy link

Added some functions simple containsfirst() and constainslast() functions and a convert function to allow Intervals to be added to Interval arrays with different types e.g.

x = Interval{Int,Closed, Open}(1,2)
containsfirst(x) # == true
containslast(x) # == false
y = Interval{Float64,Closed,Open}(1.0,2.0)
a = [x,x]
push!(a,y)

@sprockmonty sprockmonty requested a review from omus as a code owner June 19, 2020 01:46
@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #108 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   73.50%   73.44%   -0.06%     
==========================================
  Files           8        8              
  Lines         400      403       +3     
==========================================
+ Hits          294      296       +2     
- Misses        106      107       +1     
Impacted Files Coverage Δ
src/interval.jl 95.23% <66.66%> (-0.70%) ⬇️

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 401db43...a270c77. Read the comment docs.

Copy link
Collaborator

@omus omus left a comment

Choose a reason for hiding this comment

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

The new convert method looks good to me. I'm in favour of that change and I think it's best if the containsfirst/containslast change was put into a separate PR so that the convert can proceed.

src/interval.jl Outdated
@@ -137,6 +139,10 @@ function Base.convert(::Type{T}, i::Interval{T}) where T
throw(DomainError(i, "The interval is not closed with coinciding endpoints"))
end

function Base.convert(::Type{Interval{T,L,R}}, interval::Interval{N,L,R}) where {T,N,L,R}
return Interval{L,R}(convert(T, first(interval)), convert(T, last(interval)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return Interval{L,R}(convert(T, first(interval)), convert(T, last(interval)))
return Interval{T,L,R}(first(interval), last(interval))

src/interval.jl Outdated
@@ -120,6 +120,8 @@ Base.first(interval::Interval) = interval.first
Base.last(interval::Interval) = interval.last
isclosed(interval::AbstractInterval{T,L,R}) where {T,L,R} = L === Closed && R === Closed
Base.isopen(interval::AbstractInterval{T,L,R}) where {T,L,R} = L === Open && R === Open
containsfirst(interval::AbstractInterval{T,L,R}) where {T,L,R} = L === Closed
containslast(interval::AbstractInterval{T,L,R}) where {T,L,R} = R === Closed
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 this functionality is useful but the name needs more consideration. There may be a rename of first/last in the near future: #90

@sprockmonty
Copy link
Author

sprockmonty commented Jun 26, 2020

I've made the change and added an additional promote_rule function for promoting intervals.

@@ -189,6 +189,14 @@ function Base.convert(::Type{T}, interval::Interval{T}) where T
end
end

function Base.convert(::Type{<:Interval{T}}, interval::Interval{S,L,R}) where {T,S,L,R}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function Base.convert(::Type{<:Interval{T}}, interval::Interval{S,L,R}) where {T,S,L,R}
function Base.convert(::Type{Interval{T}}, interval::Interval{S,L,R}) where {T,S,L,R}

@@ -85,6 +85,7 @@ isinf(::TimeType) = false
@test_throws DomainError convert(Int, Interval{Closed, Open}(10, 10))
@test convert(Int, Interval{Closed, Closed}(10, 10)) == 10
@test_throws DomainError convert(Int, Interval{Closed, Closed}(10, 11))
@test convert(Interval{Float64, Closed, Closed}, Interval(1,2)) == Interval{Float64, Closed, Closed}(1.0,2.0)
Copy link
Collaborator

@omus omus Jul 14, 2020

Choose a reason for hiding this comment

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

Hmmm, convert(Interval{Float64, Open, Open}, Interval(1,2)) isn't lossless conversion. This means that the convert method should probably be defined as: Base.convert(::Type{Interval{T}}, interval::Interval{S,L,R}) where {T,S,L,R}

Copy link
Author

Choose a reason for hiding this comment

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

Would it also be useful to include Base.convert(::Type{Interval{T,L,R}}, interval::Interval{S}) where {T,S,L,R} such that if you need to convert from Closed Closed to e.g. Open Open you can? Should this type of conversion include Closed -> Open but not Open -> Closed

Copy link
Collaborator

@omus omus Jul 17, 2020

Choose a reason for hiding this comment

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

I think this would make more sense as a constructor: https://docs.julialang.org/en/v1/manual/conversion-and-promotion/#Conversion-vs.-Construction-1.

I'd probably leave this out unless you have a use case.

Comment on lines +110 to +112
interval1 = Interval{L1, R1}(a1, b1)
interval2 = Interval{L2, R2}(a2, b2)
@test promote(interval1, interval2) == (Interval{L1,R1}(promote(a1, b1)...), Interval{L2,R2}(promote(a2, b2)...))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
interval1 = Interval{L1, R1}(a1, b1)
interval2 = Interval{L2, R2}(a2, b2)
@test promote(interval1, interval2) == (Interval{L1,R1}(promote(a1, b1)...), Interval{L2,R2}(promote(a2, b2)...))
interval1 = Interval{L1, R1}(a1, b1)
interval2 = Interval{L2, R2}(a2, b2)
promoted1 = Interval{L1,R1}(promote(a1, b1)...)
promoted2 = Interval{L2,R2}(promote(a2, b2)...)
@test promote(interval1, interval2) == (promoted1, promoted2)

Comment on lines +196 to +198
Base.promote_rule(::Type{Interval{T,L1,R1}}, ::Type{Interval{S,L2,R2}}) where {T,S,L1,R1,L2,R2} = Interval{promote_type(T,S), <:Union{L1,L2}, <:Union{R1,R2}}
# hacky way to fix situations where T and S are the same but L and R are different
Base.not_sametype(x::Tuple{Interval{T,L1,R1}, Interval{T,L2,R2}}, y::Tuple{Interval{T,L1,R1}, Interval{T,L2,R2}}) where {T,L1,R1,L2,R2} = nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we should just define:

Base.promote_rule(::Type{Interval{T,L,R}}, ::Type{Interval{S,L,R}}) where {T,S,L,R} = Interval{promote_type(T,S), L, R}

I suspect this would eliminate the need for the hack

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.

2 participants