Add mapMaybe to sets and Seq

[ChickenProp]

As discussed in #346. We now have mapMaybe in all of Seq, Set, IntSet, strict and lazy Map, and strict and lazy IntMap. And catMaybes in all of those except IntSet (where it doesn't make sense). The issue doesn't mention all of them, but I figured it would be good to have a consistent API.

[meooow25]

Thank you for the PR! mapMaybe is welcome, but personally I don't think catMaybes is worth adding to the (already large) API. One can just write mapMaybe id for that.

@treeowl, would you agree?

[treeowl]

I also don't think we really need catMaybes, but maybe it would serve some didactic purpose? Do we want an effectful version?

traverseMaybe :: Applicative f => (a -> f (Maybe b)) -> Whatever a -> f (Whatever b)

[treeowl]

And what about WithKey variants?

[meooow25]

Do we want an effectful version?

traverseMaybe :: Applicative f => (a -> f (Maybe b)) -> Whatever a -> f (Whatever b)

That's a good question, but there's a problem since we are doing a tail-recursive construction with an accumulator. To keep the same style, we need Monad. If we want Applicative I think we would need to sacrifice some efficiency.

I'm fine with considering this later, doesn't have to be in this PR.

[ChickenProp]

Thanks for the comments, will get to them at some point.

• catMaybes: I don't have strong feelings about this. I'm not sure I've ever wanted them. For Set (and nothing else), maybe catMaybes would be more efficient than mapMaybe id (though both are O(n))? I can benchmark later if it's a crux for anything. I think two reasons to keep are

  • Consistency with Data.Maybe;
  • traverseMaybe and mapMaybeWithIndex can both be pretty easily implemented with it, so maybe it would have at least some use.

  But they're not very strong, I'm also fine with removing, and it's easier to add them later if we want them than to remove if we don't.

• traverseMaybe: agree this can be postponed.

• WithKey variants: Map and IntMap already have mapMaybeWithKey. Seq has mapWithIndex so maybe it wants mapMaybeWithIndex too. I don't think Set or IntSet have a comparable variant of map, so I wouldn't want to add it for mapMaybe.

[treeowl]

For mapMaybe for Map, shouldn't we be able to follow the map structure instead, and build up the resulting structure using join and such? How would that compare in benchmarks?

Also, I still want WithKey versions.

[ChickenProp]

mapMaybe already exists in Map. If it can be improved I'd prefer that go in a separate PR to avoid balooning this one.

Also, I still want WithKey versions.

To clarify, WithKey versions of what? The only place I'd expect functions named mapMaybeWithKey are in Map and IntMap, and those already have them.

[treeowl]

Sorry, I seem to have half-remembered some bits. I need to look through again.

[ChickenProp]

I think I've resolved all the comments above, except I didn't add mapMaybeWithIndex. I realized it's kinda weird, since an element's index might have changed by the time we get to it - do we want "the index it had in the original Seq" or "the index it will have, if it gets kept"? And I don't think anyone was explicitly asking for it, anyway.

[ChickenProp]

For Set (and nothing else), maybe catMaybes would be more efficient than mapMaybe id (though both are O(n))? I can benchmark later if it's a crux for anything.

I did do a benchmark, and a dedicated catMaybes can indeed be more efficient. With this diff:

diff --git a/containers-tests/benchmarks/Set.hs b/containers-tests/benchmarks/Set.hs
index 9e367391..6708cba6 100644
--- a/containers-tests/benchmarks/Set.hs
+++ b/containers-tests/benchmarks/Set.hs
@@ -7,6 +7,7 @@ import Control.Exception (evaluate)
 import Test.Tasty.Bench (bench, bgroup, defaultMain, whnf)
 import Data.List (foldl')
 import qualified Data.Set as S
+import qualified Data.Set.Internal as S
 
 import Utils.Fold (foldBenchmarks)
 import Utils.Random (shuffle)
@@ -16,6 +17,7 @@ main = do
         s_even = S.fromList elems_even :: S.Set Int
         s_odd = S.fromList elems_odd :: S.Set Int
         strings_s = S.fromList strings
+        s_maybe = S.insert Nothing $ S.map Just s
     evaluate $ rnf [s, s_even, s_odd]
     evaluate $ rnf
       [elems_distinct_asc, elems_distinct_desc, elems_asc, elems_desc]
@@ -68,6 +70,8 @@ main = do
         , bench "eq" $ whnf (\s' -> s' == s') s -- worst case, compares everything
         , bench "compare" $ whnf (\s' -> compare s' s') s -- worst case, compares everything
         , bgroup "folds" $ foldBenchmarks S.foldr S.foldl S.foldr' S.foldl' foldMap s
+        , bench "catMaybes1" $ whnf S.catMaybes1 s_maybe
+        , bench "catMaybes2" $ whnf S.catMaybes2 s_maybe
         ]
   where
     bound = 2^12
diff --git a/containers/src/Data/Set/Internal.hs b/containers/src/Data/Set/Internal.hs
index 0af7eb15..a330491f 100644
--- a/containers/src/Data/Set/Internal.hs
+++ b/containers/src/Data/Set/Internal.hs
@@ -163,6 +163,8 @@ module Data.Set.Internal (
             , dropWhileAntitone
             , spanAntitone
             , mapMaybe
+            , catMaybes1
+            , catMaybes2
             , partition
             , split
             , splitMember
@@ -231,6 +233,7 @@ module Data.Set.Internal (
 
 import Utils.Containers.Internal.Prelude hiding
   (filter,foldl,foldl',foldr,null,map,take,drop,splitAt)
+import qualified Data.Maybe as Maybe
 import Prelude ()
 import Control.Applicative (Const(..))
 import qualified Data.List as List
@@ -1011,6 +1014,13 @@ mapMaybe f t = finishB (foldl' go emptyB t)
           Just x' -> insertB x' b
 {-# INLINABLE mapMaybe #-}
 
+catMaybes1, catMaybes2 :: Ord a => Set (Maybe a) -> Set a
+{-# INLINABLE catMaybes1 #-}
+{-# INLINABLE catMaybes2 #-}
+
+catMaybes1 = mapMaybe id
+catMaybes2 = mapMonotonic Maybe.fromJust . dropWhileAntitone Maybe.isNothing
+
 {----------------------------------------------------------------------
   Map
 ----------------------------------------------------------------------}

I get

$ cabal run set-benchmarks -- -p catMaybes +RTS -T
Configuration is affected by the following files:
- cabal.project
All
  catMaybes1: OK
    51.4 μs ± 4.1 μs, 353 KB allocated, 7.0 KB copied, 8.0 MB peak memory
  catMaybes2: OK
    30.2 μs ± 2.7 μs, 160 KB allocated, 3.1 KB copied, 8.0 MB peak memory

I still don't feel strongly about including it.

[meooow25]

Thank you, just a couple of minor comments.

I did do a benchmark, and a dedicated catMaybes can indeed be more efficient.

That's a good point about Set, since Nothing will always be the minimum value.

I still don't feel strongly about including it.

I agree, if someone really wants this we can consider adding it later.

[ChickenProp]

Thanks, updated.

[meooow25]

Looks good, thanks!