Add filterKeys for Map and IntMap

[flip111]

#866

[treeowl]

Please don't include your Stack files. While I won't require it, I would prefer to see validity tests and also property tests (with validity tests) added both for filterKeys and filterWithKey. Also, CI is failing.

[flip111]

Hello. The stack files i committed accidentally in my last commit. I will try to fix the test again

[meooow25]

We have

filter p m = filterWithKey (\_ x -> p x) m

So why not

filterKeys p m = filterWithKey (\k _ -> p k) m

instead of duplicating the definition of filterWithKey?

[konsumlamm]

I don't see the advantage of this over just using filterWithKey?

[flip111]

It's the equivalent of filter but then for keys

[konsumlamm]

Sure, I know what it does, but why should we add it, when we can just use filterWithKey, which isn't any slower, instead?

[flip111]

I can't speak for the original poster @emilypi but for my point of view is that haskell libraries have many convenience functions that potentially can be expressed in terms of extra functions. With filterWithKey you have an additional argument that is not used (when using it like filterKeys).

[emilypi]

@flip111 that's exactly it. filterKeys is something many people do, and so we should optimize for it. The point is convenience. It's very low cost to add it, and i would argue it meets my personal Fairbarn threshold due to the fact that every other container library in existence features it.

[konsumlamm]

every other container library in existence features it.

Really? I don't think I've ever seen it anywhere, could you give some examples?

[emilypi]

Some examples:

• guava
• kotlin
• scala - alternatively, view the deprecated version which used to act on the map itself.
• purescript
• clojure - they have a general policy of only supplying O(1) functions in the core libraries (one would have to issue (apply dissoc <f> <map>), but alternative cores supply this like the ones linked.
• C++ experimental - there are many stack overflow questiosn regarding this function, and the current state of the non-experimental art is to write a for-loop which removes keys iteratively using std::map::erase.

There are many more examples, but I don't want to spend too much time on it. I don't see a valid argument against this other than "why not use filterWithKey", which has been addressed, and is largely a subjective for both sides. I've written this function alot, and I'd like to stop the repetition 😄

[PPKFS]

This seems like something that:

• is useful
• is no added overhead
• is not some obscurely named and cryptic to understand function
• sure, it can be done already with filterWithKey or some equivalent but writing filterWithKey (f . snd) feels very strange - or should we be working in favour of removing every function that is just a more specific form of another function?

[Melvar]

I wanted this function a couple weeks ago. Of course I just used filterWithKey, but I had to check for the absence of filterKeys first.

[treeowl]

There seem to be enough people who want this to overcome my general reluctance to add semi-unnecessary functions to this package. I want some documentation and testing tweaks for both versions, even though I only added line comments to one.

[treeowl]

We seem to be missing property tests for filterWithKey and filter as well as the new function. The one for filterWithKey should look something like this:

prop_filterWithKey :: Fun (Int, Int) Bool -> IMap -> Property
prop_filterWithKey fun m =
  valid m' .&&. toList m' === Prelude.filter (apply fun) (toList m)
  where
    m' = filterWithKey (apply2 fun) m

The others should be similar, with appropriate tweaks.

[emilypi]

If we implement filterKeys in terms of filterWithKey, that'd be great, but if we end up with the duplicate definition we currently have, I'd also want some tests for fidelity between the two a la

prop_filterKeysFidelity :: Fun Int Bool -> IMap -> Property
prop_filterKeysFidelity p m = fwk === fk 
  where
    fwk = filterWithKey (\k _ -> apply p k) m
    fk = filterKeys (apply p) m

[flip111]

@treeowl @emilypi any more comments? I made some changes after your review.

[emilypi]

nice :)

[emilypi]

Looks fine to me, but it's up to @treeowl

[flip111]

@treeowl could you take a look please? Is there anything i can do or something that needs to be done?

[meooow25]

@flip111 would you like to finish the work here?

[flip111]

Sorry i didn't do this earlier, sometimes things seem like a mountain when it's actually not that hard. It went a bit out of focus, thanks for the reminder @meooow25

[flip111]

I guess now, sync with upstream master, rebase and adjust changelog?

[meooow25]

Thanks, just a few more comments and note the test build failure.

I guess now, sync with upstream master, rebase and adjust changelog?

Sure, you can do that if you like.

[meooow25]

I believe treeowl is asking for something like this instead

containers/containers/src/Data/Map/Internal.hs

Lines 1516 to 1518 in b24068b

	-- @
	-- take n = 'fromDistinctAscList' . 'Prelude.take' n . 'toAscList'
	-- @

Which in this case is simply the definition.

-- @
-- filterKeys p = 'filterWithKey' (\k _ -> p k)
-- @

Also please do the same for Map.

[flip111]

I had put that there as example for the documentation for the users. Shall i not just keep that? I'm not familiar with @ in a docblock, is it a test? I can add those tests and at the same time keep prop_filterKeys from intmap-properties and map-properties

[meooow25]

No it's not a test, just plain documentation. It will look like this: https://hackage.haskell.org/package/containers-0.7/docs/Data-Map-Lazy.html#v:take

Anyway, you've added it below so please remove this line "> filterKeys (> 4) == filterWithKey (\k _ -> k > 4)".

[meooow25]

No it's not a test, just plain documentation. It will look like this: https://hackage.haskell.org/package/containers-0.7/docs/Data-Map-Lazy.html#v:take

Anyway, you've added it below so please remove this line "> filterKeys (> 4) == filterWithKey (\k _ -> k > 4)".