Encode bound information as type parameter #102

omus · 2020-06-10T13:37:53Z

When updating the Intervals.jl to support unbounded ranges I encoutered an issue where the interface we had made it difficult to define unbounded ranges. For example if we update Inclusivity to have unbounded fields then constructor would take 4 bools: left open/closed, right open/closed, left unbounded, right unbounded:

Interval(1, 2, Inclusivity(false, true, true, false))  # `(,2]`

(Aside: note we still store a value for the unbounded side as we want Interval to be a bits type. The value here ultimately is ignored as it is unbounded)

This interface isn't very intuitive and needs some revision. While looking into reducing storage size of intervals I discovered that we can reduce the size of an interval in memory by having the bound information encoded in the the type parameters. The clearest way to do this is to use symbols. Using the same example as above we could have:

Interval{:unbounded, :closed}(1, 2)

With the option to alternatively construct the same interval with in the future:

Interval(nothing, 2)

This PR currently implements minimal working changes that support encoding bound information as type parameters. At this point there is no support for unbounded ranges. If we like this interface I can update the remainder of the code. If we don't like this I could work on an alternative interface such as:

Interval(1, 2, :unbounded, :closed)

omus · 2020-06-10T13:41:35Z

Documentation is currently causing test failures

iamed2 · 2020-06-10T13:52:14Z

I would suggest making them singleton struct types instead of symbols, since I think they're easier for the compiler to optimize out. Plus then you could have:

abstract type Endpoint end
abstract type Bounded <: Endpoint end
struct Unbounded <: Endpoint end
struct Open <: Bounded end
struct Closed <: Bounded end

and then we could define methods which only work on bounded intervals more cleanly. We could also have:

struct AnchoredInterval{P, T, L, R} where {L<:Bounded, R<:Bounded} <: AbstractInterval{T, L, R}

iamed2 · 2020-06-10T13:55:41Z

I like this overall. It's nice to be able to say const ClosedInterval = Interval{T, Closed, Closed} or const Everything = Interval{T, Unbounded, Unbounded}.

Plus it seems unlikely that you'll want to store packed arrays with different inclusivities, which is the thing you need to worry about when adding type parameters.

omus · 2020-06-10T14:13:06Z

I would suggest making them singleton struct types instead of symbols, since I think they're easier for the compiler to optimize out. Plus then you could have:
abstract type Endpoint end
abstract type Bounded <: Endpoint end
struct Unbounded <: Endpoint end
struct Open <: Bounded end
struct Closed <: Bounded end
and then we could define methods which only work on bounded intervals more cleanly. We could also have:
struct AnchoredInterval{P, T, L, R} where {L<:Bounded, R<:Bounded} <: AbstractInterval{T, L, R}

I can give this a try again. In a previous incarnation I wanted to do this and also use symbols in constructors as a convenient syntax but I ran into some issues. I filed an issue on this: JuliaLang/julia#36213

codecov · 2020-06-10T14:14:50Z

Codecov Report

Merging #102 into master will decrease coverage by 26.30%.
The diff coverage is 42.22%.

@@             Coverage Diff             @@
##           master     #102       +/-   ##
===========================================
- Coverage   96.25%   69.94%   -26.31%     
===========================================
  Files           8        8               
  Lines         267      376      +109     
===========================================
+ Hits          257      263        +6     
- Misses         10      113      +103

Impacted Files	Coverage Δ
src/deprecated.jl	`1.85% <0.00%> (-38.15%)`	⬇️
src/description.jl	`100.00% <ø> (ø)`
src/Intervals.jl	`100.00% <100.00%> (ø)`
src/anchoredinterval.jl	`98.21% <100.00%> (ø)`
src/endpoint.jl	`93.54% <100.00%> (+0.44%)`	⬆️
src/inclusivity.jl	`100.00% <100.00%> (ø)`
src/interval.jl	`97.19% <100.00%> (-0.06%)`	⬇️
src/plotting.jl	`94.11% <100.00%> (ø)`

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 91e7474...f24a406. Read the comment docs.

iamed2 · 2020-06-10T14:18:29Z

I commented on that issue. I don't think your use of type parameters there was sound.

nickrobinson251 · 2020-06-10T14:23:04Z

I would suggest making them singleton struct types

Is there any reason to have some singletons as structs? Can they all be abstract types?

omus · 2020-06-10T14:24:46Z

I would suggest making them singleton struct types

Is there any reason to have some singletons as structs? Can they all be abstract types?

Making them struct makes them leaf types. I don't expect we'd want to make subtypes of those bounds.

iamed2 · 2020-06-10T14:26:11Z

It also means we can instantiate them, which can be easier for passing and dispatching on argument types. All traits in base Julia are like that.

nickrobinson251 · 2020-06-10T14:29:48Z

It seems a little wonky to me to be able to write Unbounded() but not Bounded() (and/or to mix dispatching on ::Type{T} with dispatching on ::T).

omus · 2020-06-10T15:02:13Z

I should note that Endpoint is already a type so we'll end up with this for the initial version which doesn't have Unbounded support:

abstract type Bound end
struct Open <: Bound end
struct Closed <: Bound end

When including Unbounded we could have:

abstract type Bound end
abstract type Bounded <: Bound end
struct Unbounded <: Bound
struct Open <: Bounded end
struct Closed <: Bounded end

I think I'd probably leave out Bounded to start with.

omus · 2020-06-10T15:05:50Z

I should also point out that IntervalSets.jl uses Interval{L,R,T} where L and R are symbols. I think Interval{T,L,R} is a better ordering as I expect Interval{T} will be used more often when it comes to dispatch. Additionally this order happens to be non-breaking for Intervals.jl but it is still good to discuss our ideal parametric ordering.

omus · 2020-06-10T18:17:58Z

As the response to this proposed change has been positive I'll complete the remainder of the work:

Add deprecation warnings
Update code to no longer use deprecated functions

src/Intervals.jl

omus · 2020-06-11T14:12:08Z

PR should now be complete and can be reviewed at this time. I'll call out the couple of minor negative points to do with this change:

Interval{Open, Closed}(1, 2) works as you'd expect but Interval{Open, Closed} won't get you the type you expect. Instead you need to write Interval{T,Open,Closed} where T
The HE and HB constructors have no way of supplying an alternative bound types. I don't believe this an issue and HourEnding{L,R}(...) and HourBeginning{L, R}(...) works as you expect (Note: the HE and HB perform ceil/floor to the nearest hour)

fchorney · 2020-06-11T16:10:39Z

src/anchoredinterval.jl

 end

-function Base.convert(::Type{Interval{T}}, interval::AnchoredInterval{P, T}) where {P, T}
-    return Interval{T}(first(interval), last(interval), inclusivity(interval))
+function Base.convert(::Type{Interval{T}}, interval::AnchoredInterval{P,T,L,R}) where {P,T,L,R}


Is there a situation where this function is used over the one above? or more so, would there ever be a difference?

The signatures of these two functions is identical with the updated AnchoredInterval type.

Do we need them both then?

I misinterpreted your original question. The actual answer to your initial question is that there is a difference between the two functions. One is called as convert(Interval, ...) which is just "convert this to a subtype of Interval" where convert(Interval{Int64}, ...) is "convert this to a subtype of Interval where the element type is Int64".

We could combine these so the signature is:

Base.convert(::Union{Type{Interval}, Type{Interval{T}}, ...)

I may make that change in another PR.

I think this is just me trying to wrap my head around multiple dispatch for certain things. I get that one is just strictly Interval and the other is Interval{T}, but doesn't the Interval{T} one stipulate that the T has to be the same type as the T in the AnchoredInterval{P,T,L,R}?, and therefore you wouldn't be able to do something like convert(Interval{Float64}) if the T in the AnchoredInterval was an Int64 per se? Perhaps I am misunderstanding, and both T's don't actually need to match?

src/anchoredinterval.jl

src/description.jl

nickrobinson251 · 2020-06-11T17:52:13Z

The HE and HB constructors have no way of supplying an alternative bound types

🤷 Different issue, but HE and HB are the least nice interface in this package already #105

omus · 2020-06-11T19:06:38Z

Just applied Fernando's change and corrected the doctests

fchorney

LGTM

omus mentioned this pull request Jun 10, 2020

Unbounded interval support #104

Merged

iamed2 reviewed Jun 10, 2020

View reviewed changes

src/Intervals.jl Outdated Show resolved Hide resolved

omus force-pushed the cv/parametric-bounds branch from 91939f4 to 11f70ed Compare June 11, 2020 14:07

omus changed the title ~~WIP: Encode bound information as type parameter~~ Encode bound information as type parameter Jun 11, 2020

fchorney reviewed Jun 11, 2020

View reviewed changes

omus force-pushed the cv/parametric-bounds branch from 88f26f6 to 70443cb Compare June 11, 2020 18:53

fchorney approved these changes Jun 11, 2020

View reviewed changes

omus added 6 commits June 11, 2020 23:59

Minimal changes for bounds in type parameters

d4775b3

No spaces when using simple type parameters

3eef81e

Minimal changes to documentation

349e7c0

Use types for bounds

df6a902

Add deprecation warnings

da0a048

Update deprecated constructor calls

e561ad9

omus force-pushed the cv/parametric-bounds branch from 70443cb to a59deb6 Compare June 12, 2020 05:07

omus added 2 commits June 12, 2020 00:13

Update deprecated Inclusivity calls

8cfea90

Rename bound/bounds to bound_type/bounds_types

f24a406

omus force-pushed the cv/parametric-bounds branch from a59deb6 to f24a406 Compare June 12, 2020 05:16

omus merged commit 78b125a into master Jun 12, 2020

omus deleted the cv/parametric-bounds branch June 12, 2020 12:33

omus mentioned this pull request Jul 12, 2021

Order of type parameters is somewhat problematic #170

Open

omus mentioned this pull request May 26, 2023

[WIP] Interval bounds field #217

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode bound information as type parameter #102

Encode bound information as type parameter #102

omus commented Jun 10, 2020

omus commented Jun 10, 2020

iamed2 commented Jun 10, 2020 •

edited

Loading

iamed2 commented Jun 10, 2020

omus commented Jun 10, 2020

codecov bot commented Jun 10, 2020 •

edited

Loading

iamed2 commented Jun 10, 2020

nickrobinson251 commented Jun 10, 2020

omus commented Jun 10, 2020

iamed2 commented Jun 10, 2020

nickrobinson251 commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus commented Jun 11, 2020

fchorney Jun 11, 2020

omus Jun 11, 2020

fchorney Jun 11, 2020

omus Jun 12, 2020 •

edited

Loading

fchorney Jun 12, 2020 •

edited

Loading

nickrobinson251 commented Jun 11, 2020

omus commented Jun 11, 2020

fchorney left a comment

Encode bound information as type parameter #102

Encode bound information as type parameter #102

Conversation

omus commented Jun 10, 2020

omus commented Jun 10, 2020

iamed2 commented Jun 10, 2020 • edited Loading

iamed2 commented Jun 10, 2020

omus commented Jun 10, 2020

codecov bot commented Jun 10, 2020 • edited Loading

Codecov Report

iamed2 commented Jun 10, 2020

nickrobinson251 commented Jun 10, 2020

omus commented Jun 10, 2020

iamed2 commented Jun 10, 2020

nickrobinson251 commented Jun 10, 2020 • edited Loading

omus commented Jun 10, 2020 • edited Loading

omus commented Jun 10, 2020 • edited Loading

omus commented Jun 10, 2020 • edited Loading

omus commented Jun 11, 2020

fchorney Jun 11, 2020

Choose a reason for hiding this comment

omus Jun 11, 2020

Choose a reason for hiding this comment

fchorney Jun 11, 2020

Choose a reason for hiding this comment

omus Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

fchorney Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

nickrobinson251 commented Jun 11, 2020

omus commented Jun 11, 2020

fchorney left a comment

Choose a reason for hiding this comment

iamed2 commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 10, 2020 •

edited

Loading

nickrobinson251 commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus commented Jun 10, 2020 •

edited

Loading

omus Jun 12, 2020 •

edited

Loading

fchorney Jun 12, 2020 •

edited

Loading