Add open and mixed open/closed intervals #26

dlfivefifty · 2018-03-19T15:30:15Z

This PR adds support for general closed/open endpoints.

It also makes the following changes which I believe are necessary:

union(a .. b, nextfloat(b) .. c) now throws an error (for a < b < c): this is consistent with the fact that union(0 .. 1, 2..3) should not be 0 .. 3, and because

a, b, c = 0.0, 1.0, 2.0
x = nextfloat(BigFloat(b))
!(x in a..b) && !(x in nextfloat(b)..c)

Previously x in union(a .. b, nextfloat(b) .. c) returned true, which is inconsistent with the above!
2. convert(UnitRange, 0..1) is now deprecated in favour of UnitRange(0..1), as convert should throw an error (e.g. convert(Int, 1+Im) throws an error).

dlfivefifty · 2018-03-21T15:12:52Z

@timholy This is ready for review.

dlfivefifty · 2018-07-28T09:27:40Z

@timholy @ararslan Could this be looked at, please? I need this in the in-progress update to ApproxFun .

ararslan

Seems okay to me other than the couple of things I noted.

ararslan · 2018-07-28T19:30:22Z

src/IntervalSets.jl

@@ -4,29 +4,73 @@ module IntervalSets

 using Base: @pure
 import Base: eltype, convert, show, in, length, isempty, isequal, issubset, ==, hash,
-             union, intersect, minimum, maximum, extrema, range, ⊇
+             union, intersect, minimum, maximum, extrema, range, ⊇, mean, median


In Julia 0.7, mean and median are in the Statistics stdlib package. So instead of adding them here, you could add

import Compat.Statistics: mean, median

Just make sure that REQUIRE has an appropriate lower bound for Compat in that case.

ararslan · 2018-07-28T19:33:09Z

src/IntervalSets.jl

+end
+
+mean(d::AbstractInterval) = one(eltype(d))/2 * (leftendpoint(d) + rightendpoint(d))
+median(d::AbstractInterval) = mean(d)


I'm not sure about this definition. Typically (not always, granted) the median of some set is an element of the set, but that isn't guaranteed when using a mean. Maybe this is fine, I dunno.

ararslan · 2018-07-28T19:33:53Z

src/IntervalSets.jl

+    b
+end
+
+mean(d::AbstractInterval) = one(eltype(d))/2 * (leftendpoint(d) + rightendpoint(d))


Is the one part needed here? Couldn't you just write this as

(leftendpoint(d) + rightendpoint(d)) / 2

?

…ically for this • AbstractInfiniteSet -> Domain • deprecate length in favour of duration

dlfivefifty · 2018-07-31T10:26:41Z

@ararslan I've addressed your comments, and just removed median as I don't see any use for it. I've also made the following changes which you might have some opinions on:

I renamed AbstractInfiniteSet as Domain, as AbstractSet has a (currently undefined) meaning in Julia, which may implicitly imply that the set is countable. I'd be fine removing this abstract type completely.
Added abstract type TypedEndpointsInterval{L,R,T} to allow custom implementations of intervals such as MyClosedUnitInterval.
Deprecated length in favour of duration, to address Rename length to width/duration #31. This PR already implemented width for arclength.

ararslan · 2018-07-31T16:33:55Z

Looks good to me, but I'm not a primary maintainer of this package, so it'd be good to hear from someone else as well before merging.

dlfivefifty · 2018-07-31T16:34:53Z

We can always chat next week about it...

dlfivefifty · 2018-07-31T19:42:19Z

Is leftendpoint/rightendpoint too verbose? Would left and right be better?

ararslan · 2018-07-31T19:43:11Z

Personally I prefer the clarity of (left|right)endpoint; just left and right seem overly general, IMO.

dlfivefifty · 2018-08-16T08:13:46Z

@timholy I mentioned this PR at JuliaCon. Do you have time to review it now?

(Still no rush on my part but a revamp of domains in ApproxFun.jl is waiting on this.)

timholy

Sorry I forgot about this. It looks fantastic! Tiny comments, neither of which is necessary.

src/IntervalSets.jl

src/interval.jl

timholy · 2018-09-01T11:38:50Z

In other words, merge at will. If you are (justifiably) annoyed by the delay, I can help fix those merge conflicts. Just let me know.

dlfivefifty · 2018-09-01T13:39:11Z

That's OK, I've resolved the conflicts, though you might have comments on the changes I made:

I removed the general AbstractUnitRange conversion because it seemed to broad, in favour of specific ones:

UnitRange{T}(i::TypedEndpointsInterval{:closed,:closed,I}) where {T<:Integer,I<:Integer} = UnitRange{T}(minimum(i), maximum(i))
UnitRange(i::TypedEndpointsInterval{:closed,:closed,I}) where {I<:Integer} = UnitRange{I}(i)

I added convert(AbstractInterval, x::Number) and variants
I overrode issubset, as the existing definition called the now deprecated length.

timholy · 2018-09-01T14:39:13Z

On my phone. See my recent PR to Base re issubset. Can't really check rest now, you are authorized to decide😀

…

On Sat, Sep 1, 2018, 8:39 AM Sheehan Olver ***@***.***> wrote: That's OK, I've resolved the conflicts, though you might have comments on the changes I made: 1. I removed the general AbstractUnitRange conversion because it seemed to broad, in favour of specific ones: UnitRange{T}(i::TypedEndpointsInterval{:closed,:closed,I}) where {T<:Integer,I<:Integer} = UnitRange{T}(minimum(i), maximum(i))UnitRange(i::TypedEndpointsInterval{:closed,:closed,I}) where {I<:Integer} = UnitRange{I}(i) 1. I added convert(AbstractInterval, x::Number) and variants 2. I overrode issubset, as the existing definition called the now deprecated length. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdG6e1ZaYls6XNKD2UV4ZIUspZDGOLvks5uWo3_gaJpZM4SwYH_> .

timholy · 2018-09-03T23:14:09Z

Having looked again I think your taste in these matters was exquisite. I really like the distinction you made in the comments between conversion and construction, I'm going to be more deliberate about that in the future.

The override for issusbset will become complicated once the backports happen for 0.7, but that's just how it goes.

hyrodium · 2022-04-02T12:42:19Z

src/IntervalSets.jl

+# convert numbers to intervals
+convert(::Type{AbstractInterval}, x::Number) = x..x
+convert(::Type{AbstractInterval{T}}, x::Number) where T =
+    convert(AbstractInterval{T}, convert(AbstractInterval, x))
+convert(::Type{TypedEndpointsInterval{:closed,:closed}}, x::Number) = x..x
+convert(::Type{TypedEndpointsInterval{:closed,:closed,T}}, x::Number) where T =
+    convert(AbstractInterval{T}, convert(AbstractInterval, x))
+convert(::Type{ClosedInterval}, x::Number) = x..x
+convert(::Type{ClosedInterval{T}}, x::Number) where T =
+    convert(AbstractInterval{T}, convert(AbstractInterval, x))


Do we need these conversion methods? x-ref:#97

dlfivefifty and others added 8 commits March 18, 2018 21:19

Start open interval

b53b618

using works

5d650de

Fix type inferrence so old tests pass

45f46b0

Tests pass

f9ac295

Fix warning in 0.7

14f712a

Improve coverage

39a2e25

Add test for custom defined interval conversion

806eee4

last few coverage lines

ac75a68

dlfivefifty mentioned this pull request Mar 19, 2018

Overlapping packages JuliaApproximation/DomainSets.jl#14

Closed

dlfivefifty and others added 4 commits March 19, 2018 20:02

Last few lines, hopefully

7cfd8c0

broken test fixed

1dce0a6

Remove dodgy convert routine

43d2d95

Add to README, add mixed type intersect (needs tests still)

dc82d03

ararslan requested a review from timholy March 21, 2018 19:08

ararslan reviewed Jul 28, 2018

View reviewed changes

dlfivefifty added 2 commits July 31, 2018 11:17

• Add TypedEndpointsInterval, implement union, intersect, etc. specif…

ca756d5

…ically for this • AbstractInfiniteSet -> Domain • deprecate length in favour of duration

update Compat, remove median, update mean

d506f8d

export isclosed, simplify default convert to avoid ambiguities

9ed52a6

dlfivefifty mentioned this pull request Jul 31, 2018

Illegal inttoptr in ApproxFun branch at compile time JuliaLang/julia#28379

Closed

timholy reviewed Sep 1, 2018

View reviewed changes

src/IntervalSets.jl Outdated Show resolved Hide resolved

src/interval.jl Outdated Show resolved Hide resolved

address timholy's comments

b3e6310

dlfivefifty added 2 commits September 1, 2018 14:15

merge

3f6111f

update issubset, add conversion of numbers to intervals

179646f

issubset override dependent on VERSION

f1f346e

dlfivefifty merged commit a0983f1 into JuliaMath:master Sep 1, 2018

dlfivefifty deleted the dl/open branch September 1, 2018 19:34

hyrodium reviewed Apr 2, 2022

View reviewed changes

hyrodium mentioned this pull request Apr 3, 2022

Add deprecated message for conversions number to interval #98

Merged

Add open and mixed open/closed intervals #26

Add open and mixed open/closed intervals #26

Uh oh!

Conversation

dlfivefifty commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlfivefifty commented Mar 21, 2018

Uh oh!

dlfivefifty commented Jul 28, 2018

Uh oh!

ararslan left a comment

Choose a reason for hiding this comment

Uh oh!

ararslan Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

ararslan Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

ararslan Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

dlfivefifty commented Jul 31, 2018

Uh oh!

ararslan commented Jul 31, 2018

Uh oh!

dlfivefifty commented Jul 31, 2018

Uh oh!

dlfivefifty commented Jul 31, 2018

Uh oh!

ararslan commented Jul 31, 2018

Uh oh!

dlfivefifty commented Aug 16, 2018

Uh oh!

timholy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timholy commented Sep 1, 2018

Uh oh!

dlfivefifty commented Sep 1, 2018

Uh oh!

timholy commented Sep 1, 2018 via email

Uh oh!

timholy commented Sep 3, 2018

Uh oh!

hyrodium Apr 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlfivefifty commented Mar 19, 2018 •

edited

Loading