Minimal IntervalSet type #193

rofinn · 2022-06-02T03:35:05Z

Currently added a simple IntervalSet type and most tests pass now. I've included example tests with both Vector{<:AbstractInterval} and IntervalSets to identify where they differ as union still works with both.

codecov · 2022-06-02T03:55:00Z

Codecov Report

Merging #193 (e891f30) into revert-191-revert-179-dfl/set-operations (b6272ec) will decrease coverage by 0.46%.
The diff coverage is 86.07%.

@@                             Coverage Diff                              @@
##           revert-191-revert-179-dfl/set-operations     #193      +/-   ##
============================================================================
- Coverage                                     84.63%   84.16%   -0.47%     
============================================================================
  Files                                            12       12              
  Lines                                           794      840      +46     
============================================================================
+ Hits                                            672      707      +35     
- Misses                                          122      133      +11

Impacted Files	Coverage Δ
src/Intervals.jl	`100.00% <ø> (ø)`
src/deprecated.jl	`14.87% <ø> (ø)`
src/interval_sets.jl	`91.91% <86.07%> (-3.86%)`	⬇️

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 b6272ec...e891f30. Read the comment docs.

rofinn · 2022-06-02T06:43:25Z

GitHub actions seems to be borked.

omus

First pass review

src/interval_sets.jl

omus · 2022-06-02T14:30:50Z

test/comparisons.jl


            # TODO: These functions should be compatible with unbounded intervals
            if isbounded(earlier) && isbounded(later)
-                @test union([earlier], [later]) == [expected_superset]
-                @test intersect([earlier], [later]) == [expected_overlap]
+                @test union([earlier], [later]) != [expected_superset]


Should be updated with what is expected

So we're just gonna update all these tests then? I'll also note that the union([a], [b]) method was never something that we supported prior to 1.7. I feel like the fact that the majority of this PR involves updating tests suggests they may be too verbose.

Okay, I've included both the previous expected values (which we reverted) and the current behaviour (ie: base methods) on one line to make reviewing a bit easier.

haberdashPI

would be good to add some tests in here for your use case as well

Yeah, this is a bit of a weird edge case where we'd need to test for a behaviour that wasn't defined here. I've become rather skittish of extending functions if there's concern that the new method doesn't align with the original concept/definition in Base.

I'm in agreement with @omus on this: I do see why this behavior is not something we would necessarily want to keep (it isn't really by design and I certainly didn't predict this use), and at some point this behavior should probably be deprecated. But it is the whole reason we have a breaking change that requires a revert of 1.7.0. Might as well be explicit about that in the tests, and then other folks can understand why things are set up this way for now. These tests can be marked as broken / deprecated when the change is actually made.

src/interval_sets.jl

rofinn · 2022-06-02T16:38:45Z

Thanks for the review! Folks are welcome to add commits to this branch while I tests these changes within our ecosystem to make sure we aren't missing any other breaking changes.

omus · 2022-06-02T17:52:58Z

Thanks for the review! Folks are welcome to add commits to this branch while I tests these changes within our ecosystem to make sure we aren't missing any other breaking changes.

Sounds good. I suspect I'll be making some additions next week

haberdashPI · 2022-06-02T20:33:03Z

I'd be happy to add some more tests to this branch: though I don't think I have the appropriate permissions to push here.

rofinn · 2022-06-02T20:52:02Z

Good news! Looks like these changes are no longer breaking for us, at least not in our production codebase. I'm not sure what extra tests we'd want to add as this is now often testing the base fallback behaviour we want to keep, but feel free to make a PR to this branch.

1. Made IntervalSet a subtype of AbstractSet - Note about changing internally representation once `union!(Vector{AbstractInterval})` is deprecated 2. Add an extra empty constructor 3. `==` falls back to issetequal 4. Fixed a bug where `union` didn't correctly copy data in the non-concrete case. 5. Updated a bunch of tests to include both the previous (reverted) expectation and the current base fallback behaviour.

rofinn

I'll leave this up to other folks to decide if they want add more tests.

rofinn · 2022-06-06T22:05:52Z

src/interval_sets.jl

+https://en.wikipedia.org/wiki/Interval_arithmetic#Interval_operators
+"""
+struct IntervalSet{T<:AbstractInterval} <: AbstractSet{T}
+    # TODO: Use Dict internally once union!(Vector{AbstractInterval}) is fully deprecated


In relation to a point made by @omus, we need to wrap a vector without mutating or copying in order to properly deprecated the old union! behaviour. Once that's gone we should be good to use a more appropriate internal type like a Dict.

src/interval_sets.jl

test/comparisons.jl

haberdashPI · 2022-06-17T16:25:01Z

I'm not sure I agree with the TODO about using a Dict. I think ultimately, on construction of an IntervalSet, unbunch should be called to immediately represent sets in the way that is most efficient for downstream set operations, which all use mergesets. ~~(At this point find_intersections would probably need refactoring)~~.

haberdashPI · 2022-06-17T16:33:29Z

I've added some additional proposed changes here:

#196

haberdashPI · 2022-06-17T21:56:09Z

I made some further changes over in #196, because the internal function unbunch needs to support vectors as well as sets: find_intersections should not accept IntervalSet objects, because the input is ordered, and no intervals should ever be merged, regardless of future optimizations of this kind to IntervalSet.

Tests, docs and some small fixes to IntervalSets

haberdashPI · 2022-06-24T13:02:34Z

src/interval_sets.jl

@@ -427,16 +434,17 @@ Base.intersect(x::IntervalSet, y::IntervalSet) = mergesets((inx, iny) -> inx &&
 Base.union(x::IntervalSet, y::IntervalSet) = mergesets((inx, iny) -> inx || iny, x, y)
 Base.setdiff(x::IntervalSet, y::IntervalSet) = mergesets((inx, iny) -> inx && !iny, x, y)
 Base.symdiff(x::IntervalSet, y::IntervalSet) = mergesets((inx, iny) -> inx ⊻ iny, x, y)
+Base.issubset(x::T, y::IntervalSet{<:AbstractInterval{T}}) where {T} = any(i -> x in i, y.items)


Doesn't this mean that if x doesn't have the same concrete type as the element type of y it will throw a method error? I think issubset(2, [1.0..3.0, 4.0..5.0]) should be true not a method error.

In either case we should probably add a test for this case (around line 50 of tests/sets.jl), either to verify the method error, or check that it works.

Done. One thing I also noticed is that we aren't testing things like issubset(interval, intervals) and issubset(intervals, interval) which would fallback to calling the base setdiff and erroring. I'll go through and add more tests for the other set operations in case there are other incorrect fallback conditions.

…lling back to the base `setdiff`.

haberdashPI

LGTM!

…ments.

rofinn · 2022-06-27T17:02:59Z

@omus Are you good with these changes?

src/interval_sets.jl

mattBrzezinski · 2022-06-30T16:59:19Z

src/interval_sets.jl

+    println(io, ":")
+    nrows = displaysize(io)[1]
+    half = fld(nrows, 2) - 2
+    if nrows ≥ n && half > 1


Haven't looked through the package, dunno if this is standardized in it? If not,

Suggested change

if nrows ≥ n && half > 1

if nrows >= n && half > 1

I think ≥ is pretty common in this package as it uses a lot of unicode characters.

https://github.com/invenia/Intervals.jl/blob/master/src/interval.jl#L76

src/interval_sets.jl

Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>

rofinn · 2022-06-30T18:51:27Z

Alright, I think this has been through a pretty thorough review now. I'm gonna merge and tag this, so folks can start using the new type and give feedback. I think the separate type and explicit deprecations should make it pretty unlikely to be breaking.

Introduce an IntervalSet type.

573ebe9

rofinn force-pushed the rf/intervalset-type branch from d2f7697 to 573ebe9 Compare June 2, 2022 06:39

rofinn marked this pull request as ready for review June 2, 2022 06:40

rofinn requested a review from omus as a code owner June 2, 2022 06:40

omus reviewed Jun 2, 2022

View reviewed changes

haberdashPI reviewed Jun 2, 2022

View reviewed changes

src/interval_sets.jl Outdated Show resolved Hide resolved

rofinn commented Jun 6, 2022

View reviewed changes

haberdashPI added 4 commits June 17, 2022 14:21

some small adgjuetments

a680299

update docs

707c7bf

proofreading

abaddf2

add set link

8231eeb

Merge branch 'rf/intervalset-type' into dfl/intervalset-type

4cadfe2

haberdashPI added 6 commits June 17, 2022 16:57

revise doc tests

20b0c20

support other array types for an interval set

9af3a28

remove superfluous intervalset conversion

f4386ed

fix type signature

df2d9db

fix test

3503f74

fix some type issues

fc5dea1

haberdashPI mentioned this pull request Jun 17, 2022

Initial setup beacon-biosignals/DataFrameIntervals.jl#1

Merged

3 tasks

fix bug in interval set construction

bcffe82

haberdashPI and others added 12 commits June 17, 2022 22:31

type cleanup

7f8017d

removed array-based type alias

e077615

fixed typo

3ec4910

fixed missing set alias

fe6bb91

fix method signature

f8a9a90

changed Array to convert(Array

525e59f

revisions based on review

5453749

revised show method

c170ffc

handle display size

99b5c6e

Merge pull request #196 from haberdashPI/dfl/intervalset-type

c338239

Tests, docs and some small fixes to IntervalSets

Fixed doctests for Julia 1.6 and made the IntervalSet non-iterable.

a011e00

Build documentation on 1.6 for now.

1a57132

haberdashPI suggested changes Jun 24, 2022

View reviewed changes

rofinn added 2 commits June 24, 2022 11:23

Added a few more issubset signatures that don't work because we're fa…

ca36a29

…lling back to the base `setdiff`.

Fix isssubset signatures.

40aa82b

haberdashPI approved these changes Jun 24, 2022

View reviewed changes

rofinn added 2 commits June 24, 2022 14:34

More set operation tests for mismatched Interval and IntervalSet argu…

212469e

…ments.

Fix interval set operation which were incorrectly falling back to base.

376484a

haberdashPI mentioned this pull request Jun 28, 2022

Use released version of Intervals.jl beacon-biosignals/DataFrameIntervals.jl#2

Closed

4 tasks

mattBrzezinski reviewed Jun 30, 2022

View reviewed changes

Apply suggestions from code review

e891f30

Co-authored-by: mattBrzezinski <matt.brzezinski@invenia.ca>

mattBrzezinski approved these changes Jun 30, 2022

View reviewed changes

rofinn merged commit 718ed5a into revert-191-revert-179-dfl/set-operations Jun 30, 2022

rofinn deleted the rf/intervalset-type branch June 30, 2022 18:51

rofinn mentioned this pull request Jul 4, 2022

Multirange type #182

Closed

omus mentioned this pull request Jun 14, 2023

Suppress deprecation warnings in tests #219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal IntervalSet type #193

Minimal IntervalSet type #193

rofinn commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading

rofinn commented Jun 2, 2022

omus left a comment

omus Jun 2, 2022

rofinn Jun 2, 2022 •

edited

Loading

rofinn Jun 6, 2022

haberdashPI left a comment

rofinn commented Jun 2, 2022

omus commented Jun 2, 2022

haberdashPI commented Jun 2, 2022

rofinn commented Jun 2, 2022 •

edited

Loading

rofinn left a comment

rofinn Jun 6, 2022

haberdashPI commented Jun 17, 2022 •

edited

Loading

haberdashPI commented Jun 17, 2022

haberdashPI commented Jun 17, 2022

haberdashPI Jun 24, 2022

haberdashPI Jun 24, 2022

rofinn Jun 24, 2022

haberdashPI left a comment

rofinn commented Jun 27, 2022

mattBrzezinski Jun 30, 2022

rofinn Jun 30, 2022

rofinn commented Jun 30, 2022

Minimal IntervalSet type #193

Minimal IntervalSet type #193

Conversation

rofinn commented Jun 2, 2022 • edited Loading

codecov bot commented Jun 2, 2022 • edited Loading

Codecov Report

rofinn commented Jun 2, 2022

omus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberdashPI left a comment

Choose a reason for hiding this comment

rofinn commented Jun 2, 2022

omus commented Jun 2, 2022

haberdashPI commented Jun 2, 2022

rofinn commented Jun 2, 2022 • edited Loading

rofinn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberdashPI commented Jun 17, 2022 • edited Loading

haberdashPI commented Jun 17, 2022

haberdashPI commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberdashPI left a comment

Choose a reason for hiding this comment

rofinn commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofinn commented Jun 30, 2022

rofinn commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading

rofinn Jun 2, 2022 •

edited

Loading

rofinn commented Jun 2, 2022 •

edited

Loading

haberdashPI commented Jun 17, 2022 •

edited

Loading