Add superset function #42

omus · 2018-07-14T17:28:10Z

Add superset function that creates an interval which encompasses all of the intervals within an array
~~Rename interval check functions to start with "is" (with deprecations)~~
Add Interval constructor which takes Endpoints

ararslan · 2018-07-14T19:44:12Z

src/interval.jl

+"""
+    superset(intervals::AbstractArray{<:AbstractInterval}) -> Interval
+
+Create a single interval which encompasses all of the provided intervals.


Perhaps it would be worthwhile to mention that this is the smallest such interval?

ararslan · 2018-07-14T19:45:47Z

src/interval.jl

+Create a single interval which encompasses all of the provided intervals.
+"""
+function superset(intervals::AbstractArray{<:AbstractInterval})
+    left = minimum(Intervals.LeftEndpoint.(intervals))


It should be more efficient to only make one pass over the array by providing the function as an argument to maximum and minimum, e.g.

left = minimum(LeftEndpoint, intervals) right = maximum(RightEndpoint, intervals)

(Also note that the module prefix shouldn't be necessary within the module.)

Thanks. I was developing this in the REPL which is why I have the module prefix

Looks like it isn't more efficient:

julia> @btime superset([1..2, 4..5]) # minimum(LeftEndpoint.(intervals)) 123.302 ns (5 allocations: 416 bytes) Interval{Int64}(1, 5, Inclusivity(true, true)) julia> @btime superset([1..2, 4..5]) # minimum(LeftEndpoint, intervals) 1.363 μs (17 allocations: 640 bytes) Interval{Int64}(1, 5, Inclusivity(true, true))

That's... really weird. 🤔

ararslan · 2018-07-14T19:47:17Z

src/deprecated.jl

+
+# BEGIN Intervals 0.1 deprecations
+
+@deprecate less_than_disjoint(a, b) is_less_than_disjoint(a, b) true


Would it make sense to reuse some Base terminology here and call it like isless_disjoint?

That is better. Do you think that greater_than_disjoint(a, b) should be deprecated to isless_disjoint(b, a)?

Yeah, I think that seems reasonable.

codecov · 2018-07-15T01:33:27Z

Codecov Report

Merging #42 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   98.07%   98.12%   +0.04%     
==========================================
  Files           6        6              
  Lines         208      213       +5     
==========================================
+ Hits          204      209       +5     
  Misses          4        4

Impacted Files	Coverage Δ
src/Intervals.jl	`100% <ø> (ø)`	⬆️
src/interval.jl	`96.73% <100%> (+0.18%)`	⬆️

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 171b831...b0c0b05. Read the comment docs.

omus · 2018-07-15T02:23:28Z

Tests pass on Julia 0.6. Looks like some breaking changes happened in 0.7 since #30

ararslan · 2018-07-15T05:19:29Z

Looks like that was JuliaLang/julia#27302.

omus · 2018-07-17T02:06:19Z

Rebased on top of #43 which should address the 0.7 issues

omus · 2018-07-17T14:59:16Z

32-bit issues on AppVeyor with Coverage. Intervals test all pass.

ararslan · 2018-07-17T16:19:56Z

src/interval.jl

 """
    ≪(a::AbstractInterval, b::AbstractInterval) -> Bool
-    less_than_disjoint(a::AbstractInterval, b::AbstractInterval) -> Bool


Perhaps just change the reference here to isless_disjoint?

I ended up removing the reference here as there isn't a isgreater_disjoint for the other docstring. Additionally, this specific docstring will only appear with help?> ≪ and not with help?> isless_disjoint / help?> less_than_disjoint

iamed2 · 2018-07-17T16:34:28Z

src/interval.jl

@@ -228,26 +231,26 @@ end
 Base.:⊈(a::AbstractInterval, b::AbstractInterval) = !issubset(a, b)
 Base.:⊉(a::AbstractInterval, b::AbstractInterval) = !issubset(b, a)

-function overlaps(a::AbstractInterval, b::AbstractInterval)
+function isoverlapping(a::AbstractInterval, b::AbstractInterval)


I think overlaps is easier to read and has precedent with contains as a predicate name.

Though contains is deprecated in 0.7 in favor of occursin.

Same deal with plural predicate though

I think Jeff's rule of thumb for 2-arg predicates is that it reads like English if the function is put between the arguments. To that end, a overlaps b makes sense.

I can change these back. Mainly I was thinking about these after changing less_than_disjoint to isless_disjoint and was thinking these should also start with is for consistency.

Votes are to leave this as overlaps?

iamed2 · 2018-07-17T16:35:51Z

src/interval.jl

    left = max(LeftEndpoint(a), LeftEndpoint(b))
    right = min(RightEndpoint(a), RightEndpoint(b))

    return left <= right
 end

-function contiguous(a::AbstractInterval, b::AbstractInterval)
+function iscontiguous(a::AbstractInterval, b::AbstractInterval)


I think is implies a predicate with one value. I think contiguous is fine but I don't have an issue with changing this. If you do change it, however, it should be arecontiguous.

Based upon Alex's comment should this be contiguouswith? For example a contiguouswith b.

Alternatively: adjacentto

If we were to go with the "adjacent" terminology rather than "contiguous," I think adjacent itself is a fine enough name. That said, I don't really have a problem with contiguous, but I find continugouswith to be a bit of a mouthful.

omus · 2018-07-17T17:20:07Z

I'm going to move the "is" changes to another PR as they seem more controversial (see #45)

ararslan reviewed Jul 14, 2018

View reviewed changes

omus force-pushed the cv/superset branch from 89df5fe to f646a57 Compare July 15, 2018 01:33

omus force-pushed the cv/superset branch from f646a57 to 4cd278a Compare July 17, 2018 01:58

omus force-pushed the cv/superset branch from 4cd278a to 0f29de5 Compare July 17, 2018 14:06

ararslan reviewed Jul 17, 2018

View reviewed changes

ararslan approved these changes Jul 17, 2018

View reviewed changes

iamed2 requested changes Jul 17, 2018

View reviewed changes

omus added 2 commits July 17, 2018 12:21

Add endpoint constructor for Interval

ad31bc3

Add superset

b0c0b05

omus force-pushed the cv/superset branch from 0f29de5 to b0c0b05 Compare July 17, 2018 17:24

omus mentioned this pull request Jul 17, 2018

WIP: Revisit (less|greater)_than_disjoint functions #45

Open

iamed2 approved these changes Jul 17, 2018

View reviewed changes

ararslan approved these changes Jul 17, 2018

View reviewed changes

iamed2 merged commit 6b051a1 into master Jul 17, 2018

iamed2 deleted the cv/superset branch July 17, 2018 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add superset function #42

Add superset function #42

omus commented Jul 14, 2018 •

edited

Loading

ararslan Jul 14, 2018

ararslan Jul 14, 2018

omus Jul 14, 2018

omus Jul 15, 2018

ararslan Jul 15, 2018

ararslan Jul 14, 2018

omus Jul 14, 2018

ararslan Jul 15, 2018

codecov bot commented Jul 15, 2018 •

edited

Loading

omus commented Jul 15, 2018

ararslan commented Jul 15, 2018

omus commented Jul 17, 2018

omus commented Jul 17, 2018

ararslan Jul 17, 2018

omus Jul 17, 2018 •

edited

Loading

iamed2 Jul 17, 2018

ararslan Jul 17, 2018

iamed2 Jul 17, 2018

ararslan Jul 17, 2018

omus Jul 17, 2018

omus Jul 17, 2018

iamed2 Jul 17, 2018

omus Jul 17, 2018

omus Jul 17, 2018

ararslan Jul 17, 2018

omus commented Jul 17, 2018 •

edited

Loading


		# BEGIN Intervals 0.1 deprecations

		@deprecate less_than_disjoint(a, b) is_less_than_disjoint(a, b) true

Add superset function #42

Add superset function #42

Conversation

omus commented Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2018 • edited Loading

Codecov Report

omus commented Jul 15, 2018

ararslan commented Jul 15, 2018

omus commented Jul 17, 2018

omus commented Jul 17, 2018

Choose a reason for hiding this comment

omus Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Jul 17, 2018 • edited Loading

omus commented Jul 14, 2018 •

edited

Loading

codecov bot commented Jul 15, 2018 •

edited

Loading

omus Jul 17, 2018 •

edited

Loading

omus commented Jul 17, 2018 •

edited

Loading