Revert "Revert "Multi-interval set operators""

[rofinn]

It's ugly, but necessary. We're reverting #191 which in turn reverted #179.

We just need to address the https://github.com/invenia/Intervals.jl/pull/179#issuecomment-1143001180 from the original PR and double check that no other cases exists (ie: review all new Base method overloads).

NOTE: Something else that we should probably change is the union behaviour as our definition of union doesn't really match the Base definition.

[codecov[bot]]

Codecov Report

Merging #192 (718ed5a) into master (cb4f4d5) will increase coverage by 2.43%. The diff coverage is 91.91%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   81.73%   84.16%   +2.43%     
==========================================
  Files          11       12       +1     
  Lines         624      840     +216     
==========================================
+ Hits          510      707     +197     
- Misses        114      133      +19     

Impacted Files	Coverage Δ
src/Intervals.jl	100.00% <ø> (ø)
src/deprecated.jl	14.87% <ø> (ø)
src/endpoint.jl	98.11% <ø> (ø)
src/interval.jl	96.01% <ø> (-0.35%)	:arrow_down:
src/interval_sets.jl	91.91% <91.91%> (ø)

---

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 cb4f4d5...718ed5a. Read the comment docs.

[haberdashPI]

Moving my note over:

This should also add test cases for treating each interval as an element in a set (represented by an array).

Something else that we should probably change is the union behaviour as our definition of union doesn't really match the Base definition.

Can you elaborate on this point?

[haberdashPI]

Also, happy to start making some changes here with proposed fixes. Does that work? Or did you have plans to work on this branch?

[rofinn]

Can you elaborate on this point?

Yeah, the main difference is that the Base.union is combining all unique elements of the inputs to a single collection, but the elements don't change. In our case, we're basically applying a reduction on a single collection of intervals. I think this stems from the fact that we sometimes treat intervals as scalar elements because they're non-iterable, while other times we think of them more like ranges. In any case, I don't think the term union is wrong in this case, but the behaviour is very different from the base definition, hence I think it should probably also stay within the Intervals namespace.

Also, happy to start making some changes here with proposed fixes. Does that work? Or did you have plans to work on this branch?

I was planning on making the minimal changes required to make this non-breaking and then I was gonna ask you and @omus for a review. After all I was the one complaining and reverting things :)

[omus]

I was planning on making the minimal changes required to make this non-breaking and then I was gonna ask you and @omus for a review.

Can I ask that you cherry-pick the commit from #179 and then apply your changes as a separate commit? Will be much easier to review this by seeing just the differences between that PR and this.

[rofinn]

I'd rather not mess with the history at this point. My plan was to just add an extra commit that you could review independently from all the rest.

[haberdashPI]

Best of both worlds? @rofinn if you create a separate PR that merges into this one, it would be easy to review the changes, and then we can merge into this branch, and from there to main. ?? Just a suggestion. Can also just review locally and pick the right commits for the diff.

[omus]

I'd rather not mess with the history at this point. My plan was to just add an extra commit that you could review independently from all the rest.

Can you list out what you changed as part of this PR? I've noticed that the interval_sets.jl files are named sets.jl so I'm concerned there are additional changes here that weren't present in #179

[rofinn]

History is a bit ugly, but this should be good to go now.