Implement 'intersections', and add an 'Intersection' newtype along with a 'Semigroup' instance #756
Conversation
|
I went looking for this function today as well and was a little surprised to see it was missing. |
|
The implementation should short-circuit. If the accumulator ever becomes empty, then there's no need to continue. |
|
No, that last comment was wrong. |
|
Corrected version: something like this: intersections (s0 :| ss) = foldr go id ss s0
where
go s r acc
| null acc = empty
| otherwise = r (intersection acc s) |
|
Excellent point! I'll go update my PR over on the containers repo with this.
…On Sun, Dec 6, 2020 at 10:40 AM David Feuer ***@***.***> wrote:
Corrected version: something like this:
Something like this:
intersections (s0 :| ss) = foldr go id ss s0
where
go s r acc
| null acc = empty
| otherwise = r (intersection acc s)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#756 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB56XCDD2JY6LAW3WVI32D3STPF2RANCNFSM4UPC2F2A>
.
|
|
Actually, upon second thought, the implementation suggested already short
circuits, due to the laziness properties of intersection:
intersection Tip _ = Tip
To confirm this, we can sprinkle in some trace statements:
intersections $ (trace "forcing 0" $ fromList [1,2,3]) :| [trace "forcing
1" $ fromList [4,5,6], trace "forcing 2" $ fromList [7,8,9]]
>> fromList forcing 0
>> forcing 1
>> []
On Sun, Dec 6, 2020 at 10:42 AM Reed Mullanix <[email protected]>
wrote:
… Excellent point! I'll go update my PR over on the containers repo with
this.
On Sun, Dec 6, 2020 at 10:40 AM David Feuer ***@***.***>
wrote:
> Corrected version: something like this:
>
> Something like this:
>
> intersections (s0 :| ss) = foldr go id ss s0
> where
> go s r acc
> | null acc = empty
> | otherwise = r (intersection acc s)
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#756 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB56XCDD2JY6LAW3WVI32D3STPF2RANCNFSM4UPC2F2A>
> .
>
|
|
@TOTBWF, |
|
Please be sure to document that the intersections are taken from left to right. Even aside from short-cutting, the smaller the accumulator the better. |
|
Still need to quit using |
|
Sorry about that, had a laziness brainfart! Will make the requested changes. |
|
|
||
| instance (Ord a) => Semigroup (Intersection a) where | ||
| (Intersection a) <> (Intersection b) = Intersection $ intersection a b | ||
| stimes = stimesIdempotent |
There was a problem hiding this comment.
| stimes = stimesIdempotent | |
| sconcat = coerce intersections | |
| stimes = stimesIdempotent |
| @@ -897,6 +905,24 @@ intersection t1@(Bin _ x l1 r1) t2 | |||
| {-# INLINABLE intersection #-} | |||
| #endif | |||
|
|
|||
| #if (MIN_VERSION_base(4,9,0)) | |||
| -- | The intersection of a series of sets. Intersections are performed left-to-right. | |||
| -- | Sets form a 'Semigroup' under 'intersection'. | ||
| newtype Intersection a = Intersection { getIntersection :: Set a } |
There was a problem hiding this comment.
It would be good to reference this newtype from the documentation for the union-based Semigroup Set instance.
| | null acc = empty | ||
| | otherwise = r (intersection acc s) | ||
|
|
||
| -- | Sets form a 'Semigroup' under 'intersection'. |
There was a problem hiding this comment.
The docs should mention that this type is only included for base >= 4.9.
There was a problem hiding this comment.
Better, I think: include the type and let it be useless without the Semigroup instance.
|
If we add an |
And |
|
I don't think there was consensus. There were some +1's, but a lot of arguing over whether or not If we decide that this is worth adding, then I'll make the requested changes. |
Yes, @dfeuer How do you feel about this PR? |
|
Would be nice to get this merged! |
|
Fell off my radar. Oops. |
|
Can we get this merged? It's a great addition. |
|
There seem to be a few unresolved discussions, especially regarding documentation |
|
|
Patch Description
This PR adds the following function for computing the intersection of a series of sets:
It also adds a newtype around
SetdubbedIntersection, which provides aSemigroupinstance that usesintersections.Mailing list discussion can be found here: https://mail.haskell.org/pipermail/libraries/2020-December/030951.html