Support intervals endpoints using infinity #99
Conversation
| @@ -136,46 +150,56 @@ | |||
| for i in 0:3 | |||
| interval = Interval(a, b, Inclusivity(i)) | |||
| cp = copy(interval) | |||
| lesser_val = Interval(a - unit, b + unit, Inclusivity(i)) | |||
| lesser_val = Interval(a - unit, b - unit, Inclusivity(i)) | |||
There was a problem hiding this comment.
Change isn't required but I think it makes it clearer that this interval is always less than the original
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
==========================================
+ Coverage 96.55% 96.60% +0.05%
==========================================
Files 7 7
Lines 261 265 +4
==========================================
+ Hits 252 256 +4
Misses 9 9
Continue to review full report at Codecov.
|
| right = min(RightEndpoint(a), RightEndpoint(b)) | ||
|
|
||
| return Interval(left, right) | ||
| end |
There was a problem hiding this comment.
I'll note this implementation could probably replace the intersect(::AbstractInterval{T}, ::AbstractInterval{T}) defined above this
There was a problem hiding this comment.
when you define a function with S, and T does julia enforce that it will only be called if S and T are different? or can they be the same?
There was a problem hiding this comment.
As there is no constraint between them they can be anything including the same. Since there a more specific definition where they have to be the same then this method will only be called when the are different.
| @@ -550,11 +575,11 @@ | |||
| Interval(-100, -1, Inclusivity(false, false)), | |||
| Interval(-10, -1, Inclusivity(false, false)), | |||
| Interval(10, 15, Inclusivity(false, false)), | |||
| Interval(13, 20, Inclusivity(false, false)) | |||
| Interval(13, 20, Inclusivity(false, false)), | |||
There was a problem hiding this comment.
Do we want to add infinity to the union tests? or make another union test with inf?
There was a problem hiding this comment.
Union test for infinity are included in the comparison tests
test/interval.jl
Outdated
| @@ -1,10 +1,20 @@ | |||
| Base.isinf(x::Char) = false | |||
| Base.isinf(x::TimeType) = false | |||
There was a problem hiding this comment.
could there at least be a comment?
There was a problem hiding this comment.
Definitely, it's just for tests though so it's not a big deal
There was a problem hiding this comment.
should we just define and use our own _isinf?
There was a problem hiding this comment.
I may open a PR to get isinf(::TimeType) implemented but I very much doubt I would get a PR for isinf(::Char) accepted
There was a problem hiding this comment.
If this really bothers people I can do:
_isinf(x) = isinf(x)
_isinf(::Char) = false
_isinf(::TimeType) = falseThere was a problem hiding this comment.
i'd slightly prefer that as it make the code easier to follow (to my eyes) as it make clearly we've added some particulars for our use-case
There was a problem hiding this comment.
How about another compromise?
isinf(x) = Base.isinf(x)
isinf(::Char) = false
isinf(::TimeType) = falseAvoids type piracy by declaring a new isinf
There was a problem hiding this comment.
Something similar: https://github.com/invenia/Intervals.jl/pull/104/files#r444362741. We could actually have a generic isinf implemented as:
isinf(x) = !isfinite(x) && !isnan(x)
isfinite(x) = iszero(x - x)
isnan(x) = x != xThe only issue I see with this is that the generic isnan may not always hold for all types
Adapted from #95. Takes the, hopefully, non-controversial parts of that PR and uses the
Float64infinity to validate the changes.@fchorney doing this allowed me to go very carefully through your tests. I've removed a few of them that are redundant with those which are in comparison.jl and some were re-worked to avoid code duplication.