Add difference for Map

[mschristiansen]

• Data.Map.difference
• QuickCheck tests

[fehrenbach]

I think you meant to write m1 instead of the last d on this line.

[mschristiansen]

Good catch. Fixed it. Want me to squash and force push or fine like this?

[fehrenbach]

Oh, sorry, that should indeed have been "first d".

I don't have the commit bit on this repo, but I'd assume someone would just hit the squash button.

I just added difference to -unordered-collections, fwiw.

[mschristiansen]

I had a look at the implementation in Haskell (below). There they obviously walk the tree, which I guess is far more efficient. Implementation in this PR follow the style of the other functions. Any thoughts on performance?

difference :: Ord k => Map k a -> Map k b -> Map k a
difference Tip _   = Tip
difference t1 Tip  = t1
difference t1 (Bin _ k _ l2 r2) = case split k t1 of
  (l1, r1)
    | size l1l2 + size r1r2 == size t1 -> t1
    | otherwise -> link2 l1l2 r1r2
    where
      !l1l2 = difference l1 l2
      !r1r2 = difference r1 r2

[fehrenbach]

That would certainly be a lot faster. Repeatedly calling delete does a lot of duplicate work. Traversing the trees in parallel allows you to avoid retraversing parts of the tree while searching for elements to remove, avoid allocating upper nodes that will be replaced by a later deletion, and avoid looking for elements that can't possibly be there (like {1..100} \ {200..300}).

On the other hand, O(#right * log(#left)) is not terrible.

You probably know all of this.

So sure, it would be nice if it was faster, but IMHO that should not block merging.
...not that I have anything to say on the matter...

[LiamGoodacre]

If the values of the second map aren't used then this could be typed as:
forall k v w . Ord k => Map k v -> Map k w -> Map k v

[fehrenbach]

But should it be? I think the type as it is makes sense for a binary minus operator.
Then you can't by accident subtract a Map String Int from a Map String String, holes can help you find a matching pair, etc.

Someone previously made a similar choice for unionWith :: forall k v. Ord k => (v -> v -> v) -> Map k v -> Map k v -> Map k v which could well be unionWith :: forall k a b c. Ord k => (a -> b -> c) -> Map k a -> Map k b -> Map k c.

(And I think the case for generalizing unionWith is much stronger.)

[mschristiansen]

I can't think of a case where I have used Map.difference with two maps with different value types, but can see that there could be such cases.

Also checked Haskell's containers and unordered-containers. They both use the more general type definition. E.g. Data.Map.Strict.difference

[mschristiansen]

@LiamGoodacre Good on this one?

[LiamGoodacre]

Looks good to me!

[mschristiansen]

@garyb please merge if things are looking ok to you.

[mschristiansen]

@fehrenbach merge?

[fehrenbach]

I don't have the power!

(If you don't need your maps to be ordered, difference for HashMap is in v1.3.0 of https://github.com/fehrenbach/purescript-unordered-collections)

[mschristiansen]

This PR has been open for a 1.5 months! Very frustrating for me. What must I do?

[garyb]

Thanks for working on this, and sorry it took so long to get merged, but please consider that none of us have unlimited time or are paid to do this or anything. Phrasing things the way you did in the last message is hardly the best way to make us inclined to do something, just a polite bump would have sufficed.

[mschristiansen]

@garyb Thanks for merging. You'll see above that I have tried several times to get this merged. Notified you directly 13 days ago and I have another PR on purescript-datetime that's also been sitting there long.

I know you and a few others are doing a lot of good work and there isn't quite enough of you to cover the number of libraries. Maybe give @fehrenbach the commit bit on this repo then the PR would have been merged the day after I submitted, and I would be happy to help as a reviewer.