Add filterKeys for Map and IntMap #972

flip111 · 2023-09-23T12:06:43Z

treeowl · 2023-09-27T10:39:38Z

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 · 2023-09-30T22:32:46Z

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

meooow25 · 2023-10-01T03:15:39Z

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 · 2023-12-29T12:33:06Z

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

flip111 · 2023-12-29T19:21:06Z

It's the equivalent of filter but then for keys

konsumlamm · 2023-12-29T19:34:18Z

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 · 2023-12-31T10:37:34Z

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 · 2023-12-31T16:00:00Z

@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 · 2023-12-31T16:22:00Z

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 · 2023-12-31T18:57:44Z

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 · 2023-12-31T19:14:27Z

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 · 2023-12-31T23:32:33Z

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 · 2024-01-01T02:53:18Z

containers-tests/tests/intmap-properties.hs

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.

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

containers/src/Data/IntMap/Internal.hs

flip111 · 2024-03-17T15:16:57Z

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

emilypi · 2024-03-17T20:13:57Z

containers/src/Data/IntMap/Internal.hs

+-- > filterKeys (> 4) == filterWithKey (\k _ -> k > 4)
+
+filterKeys :: (Key -> Bool) -> IntMap a -> IntMap a
+filterKeys predicate = filterWithKey (\k _ -> predicate k)


emilypi · 2024-03-17T20:14:12Z

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

flip111 · 2024-04-16T14:18:00Z

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

meooow25 · 2024-08-25T13:13:07Z

containers-tests/tests/intmap-strictness.hs

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


Did you mean to add these? Currently they are commented out.

meooow25 · 2024-08-25T13:17:22Z

containers-tests/tests/intmap-strictness.hs

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


Please move property tests to intmap-properties.hs.

meooow25 · 2024-08-25T13:19:40Z

containers-tests/tests/map-properties.hs

+test_filterKeys :: Assertion
+test_filterKeys = filterKeys (> 4) (fromList [(5,"a"), (3,"b")]) @?= singleton 5 "a"


This is not used, it needs to be added to the list of tests above.

, testCase "filterKeys" test_filterKeys

Also, it would be good to have property tests (like for IntMap).

meooow25 · 2024-08-25T13:23:46Z

containers/src/Data/Map/Internal.hs

+-- | \(O(n)\). Filter all keys that satisfy the predicate.
+--
+-- > filterKeys (> 4) (fromList [(5,"a"), (3,"b")]) == singleton 5 "a"
+


Please add a @since FIXME at the end of the docs. It will be updated by maintainers to the next release version.
Same for IntMap.

flip111 added 2 commits September 23, 2023 14:05

Add filterKeys for Map and IntMap

f030760

Fix test

c8a992f

Fix test, remove stack files

faab8f7

Using filterWithKey for filterKeys

5e14d30

treeowl requested changes Jan 1, 2024

View reviewed changes

emilypi reviewed Jan 1, 2024

View reviewed changes

containers/src/Data/IntMap/Internal.hs Outdated Show resolved Hide resolved

Changes after review

1c8ebc2

emilypi reviewed Mar 17, 2024

View reviewed changes

meooow25 requested changes Aug 25, 2024

View reviewed changes

meooow25 reviewed Aug 25, 2024

View reviewed changes

meooow25 mentioned this pull request Aug 31, 2024

Error on unused-top-binds in tests #1030

Merged

meooow25 mentioned this pull request Oct 31, 2024

Next release, 0.8 #1059

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filterKeys for Map and IntMap #972

Add filterKeys for Map and IntMap #972

flip111 commented Sep 23, 2023

treeowl commented Sep 27, 2023

flip111 commented Sep 30, 2023

meooow25 commented Oct 1, 2023

konsumlamm commented Dec 29, 2023

flip111 commented Dec 29, 2023

konsumlamm commented Dec 29, 2023

flip111 commented Dec 31, 2023

emilypi commented Dec 31, 2023

konsumlamm commented Dec 31, 2023

emilypi commented Dec 31, 2023 •

edited

Loading

PPKFS commented Dec 31, 2023

Melvar commented Dec 31, 2023

treeowl left a comment

treeowl Jan 1, 2024

emilypi Jan 1, 2024 •

edited

Loading

flip111 commented Mar 17, 2024

emilypi Mar 17, 2024

emilypi commented Mar 17, 2024

flip111 commented Apr 16, 2024

meooow25 Aug 25, 2024

meooow25 Aug 25, 2024

meooow25 Aug 25, 2024

meooow25 Aug 25, 2024

		test_filterKeys :: Assertion
		test_filterKeys = filterKeys (> 4) (fromList [(5,"a"), (3,"b")]) @?= singleton 5 "a"

Add filterKeys for Map and IntMap #972

Are you sure you want to change the base?

Add filterKeys for Map and IntMap #972

Conversation

flip111 commented Sep 23, 2023

treeowl commented Sep 27, 2023

flip111 commented Sep 30, 2023

meooow25 commented Oct 1, 2023

konsumlamm commented Dec 29, 2023

flip111 commented Dec 29, 2023

konsumlamm commented Dec 29, 2023

flip111 commented Dec 31, 2023

emilypi commented Dec 31, 2023

konsumlamm commented Dec 31, 2023

emilypi commented Dec 31, 2023 • edited Loading

PPKFS commented Dec 31, 2023

Melvar commented Dec 31, 2023

treeowl left a comment

Choose a reason for hiding this comment

treeowl Jan 1, 2024

Choose a reason for hiding this comment

emilypi Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

flip111 commented Mar 17, 2024

emilypi Mar 17, 2024

Choose a reason for hiding this comment

emilypi commented Mar 17, 2024

flip111 commented Apr 16, 2024

meooow25 Aug 25, 2024

Choose a reason for hiding this comment

meooow25 Aug 25, 2024

Choose a reason for hiding this comment

meooow25 Aug 25, 2024

Choose a reason for hiding this comment

meooow25 Aug 25, 2024

Choose a reason for hiding this comment

emilypi commented Dec 31, 2023 •

edited

Loading

emilypi Jan 1, 2024 •

edited

Loading