-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for Infinity.jl #95
Changes from 2 commits
617d1e1
4e3ebe5
6b81961
52c528a
f50c266
3564a85
2e1a039
614967f
945757d
dfb91a7
f8d3231
aba26a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,27 @@ struct Interval{T} <: AbstractInterval{T} | |
end | ||
end | ||
|
||
Interval{T}(f, l, inc::Inclusivity) where T = Interval{T}(convert(T, f), convert(T, l), inc) | ||
isbounded(a) = !isposinf(a) && !isneginf(a) | ||
isunbounded(a) = !isbounded(a) | ||
omus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function Interval{T}(f, l, inc::Inclusivity) where T | ||
if (isbounded(f) && isbounded(l)) || (isunbounded(f) && isunbounded(l)) | ||
return Interval{T}(convert(T, f), convert(T, l), inc) | ||
else | ||
# If either endpoint is unbounded, we want to convert the bounded variable, and then | ||
# try and promote them both to a compatable type. | ||
# If T is a subset of the Infinite type, then don't try to convert at all, as trying | ||
# to convert any type to Infinite will result in an error | ||
if !(T <: Infinite) | ||
if isbounded(f) | ||
f = convert(T, f) | ||
else | ||
l = convert(T, l) | ||
end | ||
end | ||
return Interval(f, l, inc) | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if a user has explicitly set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm yeah I suppose this change is actually just a byproduct of trying to throw in unbounded values into the tests. This test is specifically the issue (https://github.com/invenia/Intervals.jl/blob/fc/using-infinity/test/interval.jl#L46-L47):
When you create the interval on the left side of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test should be updated to be using: julia> promote_type(Infinite, Int)
InfExtended{Int64} Then in the tests you can use |
||
Interval{T}(f, l, x::Bool, y::Bool) where T = Interval{T}(f, l, Inclusivity(x, y)) | ||
Interval{T}(f, l) where T = Interval{T}(f, l, true, true) | ||
|
||
|
@@ -161,7 +181,8 @@ end | |
|
||
##### ARITHMETIC ##### | ||
|
||
Base.:+(a::T, b) where {T <: Interval} = T(first(a) + b, last(a) + b, inclusivity(a)) | ||
Base.:+(a::Interval, b) = Interval(first(a) + b, last(a) + b, inclusivity(a)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous version always kept the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This becomes the same issue as most of the other issues that cropped up by adding Infinity. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That conversion makes sense if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by adding the conversion: cjdoris/Infinity.jl#7 |
||
|
||
|
||
Base.:+(a, b::Interval) = b + a | ||
Base.:-(a::Interval, b) = a + -b | ||
|
@@ -338,7 +359,14 @@ function Base.merge(a::AbstractInterval, b::AbstractInterval) | |
|
||
left = min(LeftEndpoint(a), LeftEndpoint(b)) | ||
right = max(RightEndpoint(a), RightEndpoint(b)) | ||
return Interval(left, right) | ||
|
||
# This promotion fixes the situation where one endpoint has a type of | ||
# `InfExtended{T}` yet the ∞ value is of type `Infinite`. | ||
# This will cause an error when trying to `promote(left, right)` | ||
return Interval( | ||
promote(left.endpoint, right.endpoint)..., | ||
left.included, right.included | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems preferable to change the endpoint constructor ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is that you'll end up with situations such that
Thus you don't actually make it to that constructor you mention since the types are different. It ends up trying to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to also define a constructor for two endpoints where the types are different? I seems like we typically don't want to change what the Interval Type is, but with Infinity it seems to be required in certain situations
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is one option. We may alternatively want to define our own |
||
end | ||
|
||
##### TIME ZONES ##### | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,31 +18,41 @@ const INTERVAL_TYPES = [Interval, AnchoredInterval{Ending}, AnchoredInterval{Beg | |
# [12] | ||
# [45] | ||
@testset "non-overlapping" begin | ||
earlier = convert(A, Interval(1, 2, true, true)) | ||
later = convert(B, Interval(4, 5, true, true)) | ||
|
||
@test earlier != later | ||
@test !isequal(earlier, later) | ||
@test hash(earlier) != hash(later) | ||
|
||
@test isless(earlier, later) | ||
@test !isless(later, earlier) | ||
|
||
@test earlier < later | ||
@test !(later < earlier) | ||
|
||
@test earlier ≪ later | ||
@test !(later ≪ earlier) | ||
|
||
@test !issubset(earlier, later) | ||
@test !issubset(later, earlier) | ||
|
||
@test isempty(intersect(earlier, later)) | ||
@test_throws ArgumentError merge(earlier, later) | ||
@test union([earlier, later]) == [earlier, later] | ||
@test !overlaps(earlier, later) | ||
@test !contiguous(earlier, later) | ||
@test superset([earlier, later]) == Interval(1, 5, true, true) | ||
tests = [ | ||
((1, 2, true, true), (4, 5, true, true)), | ||
((-∞, 2, true, true), (4, ∞, true, true)), | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testsets have a for-loop syntax which would be better as you can output details in the name of the testset (https://docs.julialang.org/en/v1/stdlib/Test/#Test.@testset). I'd probably prefer a distinct testset named "non-overlapping unbounded" |
||
for (a, b) in tests | ||
@show a | ||
@show b | ||
@show A | ||
@show B | ||
earlier = convert(A, Interval(a[1], a[2], a[3], a[4])) | ||
later = convert(B, Interval(b[1], b[2], b[3], b[4])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These converts seem to fail for AnchoredIntervals, and I currently can't seem to figure out how to fix it. Seems to fail here https://github.com/invenia/Intervals.jl/blob/master/src/anchoredinterval.jl#L165 with the error
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this call is occuring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m unsure why that doesn’t make any sense? Seems like it would work how it used to but now has infinity values in it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason it's non-sense is the point at which you're setting the one endpoint used in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrote up another example here: #89 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok I see. Thanks for the explanation! |
||
|
||
@test earlier != later | ||
@test !isequal(earlier, later) | ||
@test hash(earlier) != hash(later) | ||
|
||
@test isless(earlier, later) | ||
@test !isless(later, earlier) | ||
|
||
@test earlier < later | ||
@test !(later < earlier) | ||
|
||
@test earlier ≪ later | ||
@test !(later ≪ earlier) | ||
|
||
@test !issubset(earlier, later) | ||
@test !issubset(later, earlier) | ||
|
||
@test isempty(intersect(earlier, later)) | ||
@test_throws ArgumentError merge(earlier, later) | ||
@test union([earlier, later]) == [earlier, later] | ||
@test !overlaps(earlier, later) | ||
@test !contiguous(earlier, later) | ||
@test superset([earlier, later]) == Interval(a[1], b[2], a[3], b[4]) | ||
end | ||
end | ||
|
||
# Compare two intervals which "touch" but both intervals do not include that point: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed as you explicitly import methods below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don’t then you can’t use the infinity symbol correctly. I suppose I could just import it with the other items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of the package I don't think we're referencing the infinity symbol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm good point I guess it's just for the tests.