Add 'fromListWithKey' to HashMap #246

josefs · 2020-01-17T15:57:58Z

No description provided.

treeowl · 2020-01-17T16:44:49Z

Can we reuse the unsafe function you wrote for unsafeInsertWith, or are there performance regressions?

josefs · 2020-01-17T17:12:37Z

Can we reuse the unsafe function you wrote for unsafeInsertWith, or are there performance regressions?

Yes, I suppose we could do that. I haven't looked in to the performance implications yet though. Will do.

josefs · 2020-01-17T17:13:40Z

Btw, the travis failure seems unrelated to my change.

josefs · 2020-01-20T11:57:04Z

Ok, so I've benchmarked the effect of having both full implementations for unsafeInsertWith and unsafeInsertWithKey compare to defining unsafeInsertWith in terms of unsafeInsertWithKey. Here are the benchmark for the former:
https://pastebin.com/s0BD8Q1G
And here are the benchmark results for defining unsafeInsertWith using unsafeInsertWithKey:
https://pastebin.com/X6FbvpZ3

The numbers change quite a bit, even for unrelated benchmarks. I assume that's due to code layout effects. If anything, the trend seems to be that it's slightly better define unsafeInsertWith using unsafeInsertWithKey rather than having two separate implementations. So I'm going going with that. Yell if you disagree @treeowl .

See haskell-unordered-containers#246 (comment) for performance numbers.

Data/HashMap/Strict/Base.hs

sjakobi

@josefs Could you address the merge conflicts?

Please also add some examples to the haddocks, ideally in doctest-style, and reference the documentation regarding the order of insertions that was added in #237.

Please also add a few tests. Unit tests will do, I think.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

See haskell-unordered-containers#246 (comment) for performance numbers.

@emilypi

Thanks to @emilypi for the suggestions

It models the test for fromListWith but makes sure that values are combined in a way that depends on the key.

josefs · 2020-06-02T15:15:23Z

I've added some more documentation and tests.

sjakobi

Thank you, @josefs! :)

I have a few more comments, but I think we're on a good track.

tests/HashMapProperties.hs

Data/HashMap/Base.hs

sjakobi · 2020-06-02T15:27:58Z

Data/HashMap/Base.hs

+-- > combine Mul = (*)
+-- > combine Add = (+)


Commutative operations make it harder to infer the behaviour of the function from the example.

Really? I thought that they would make it easier! The example is supposed to use the difference between addition and multiplication as a way to let the key affect combining the keys.

What would you suggest I use instead?

I suppose I could use something like taking the first or the last element, depending on the key.

Maybe something like

combine k x y = [ k ] ++ x ++ y

?

With a commutative operation you can't tell in which order the elements are being inserted which is an important gotcha here.

I'm not a big fan of combining the key with the values in the way you suggest. That's how it's documented in Data.Map and I specifically tried to avoid it because I think it seems very contrived.

The point about the order of operations is made in the last lines of the documentation. This mirrors the documentation of fromListWith which also has examples which uses commutative operators but then explains below the order of operations. I kind of like that structure.

Hmm, the thing is that IMHO fromListWith and fromListWithKey really have this rather serious design mistake where

fromListWith op [(k, a), (k, b)]

is not the obvious

fromList [(k, op a b)]

but instead

fromList [(k, op b a)]

I wish we could fix this properly but that's not a task for this PR.

So IMHO we need to ensure that potential users of fromListWith[Key] become aware of this gotcha ASAP so they do not produce a bug, for which I feel we, as the library authors and maintainers, would share some responsibility!

Currently the only way these haddocks hint at the gotcha is in the last two lines, which IMHO are fairly abstract and easy to overlook TLDR-style.

I think the documentation for fromListWith is less problematic because the first example with the commutative operation is very short, and the next example is very clear about the issue.

So maybe consider using David's free magma suggestion (which I feel is slightly abstract) or maybe just use Sub and Div instead of Add and Mul…

The old properties used associative operators to combine values when there were duplicate keys. With this diff we're using a non-commutative and non-associative operator which can catch more bugs.

This got lost in a rebase

treeowl · 2020-06-02T17:35:45Z

Free magmas are great for arbitrary operations.

data Magma a
  = Leaf a
  | Op (Magma a) (Magma a)

If you pull up an arbitrary HashMap a and fmap Leaf over it, then you can use Op as your combining function. Op has no structure at all, so any mistake will not get cancelled out anywhere else.

treeowl · 2020-06-02T21:58:08Z

Oh, I got mixed up. Thought you were talking about testing rather than documentation. The ergonomics are indeed quite awful. It would be much clearer if we had something like flwk :: (Hashable k, Eq k) => (k -> Maybe b -> a -> Box b) -> [(k,a)] -> HashMap k b (where data Box a = Box a) or even flwkm :: (Hashable k, Eq k) => (k -> Maybe b -> a -> Maybe b) -> [(k,a)] -> HashMap k b

…

On Tue, Jun 2, 2020, 4:31 PM Simon Jakobi ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Data/HashMap/Base.hs <#246 (comment)> : > +-- > combine Mul = (*) +-- > combine Add = (+) Hmm, the thing is that IMHO fromListWith and fromListWithKey really have this rather serious design mistake where fromListWith op [(k, a), (k, b)] is not the obvious fromList [(k, op a b)] but instead fromList [(k, op b a)] I wish we could fix this properly but that's not a task for this PR. So IMHO we need to ensure that potential users of fromListWith[Key] become aware of this gotcha ASAP so they *do not* produce a bug, for which I feel we, as the library authors and maintainers, would share some responsibility! Currently the only way these haddocks *hint* at the gotcha is in the last two lines, which IMHO are fairly abstract and easy to overlook TLDR-style. I think the documentation for fromListWith is less problematic because the first example with the commutative operation is very short, and the next example is very clear about the issue. So maybe consider using David's free magma suggestion (which I feel is *slightly* abstract) or maybe just use Sub and Div instead of Add and Mul… — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#246 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOF7NWYF25GCDRVCZA6UDRUVOSTANCNFSM4KIKMSJQ> .

sjakobi · 2020-06-02T22:34:47Z

@treeowl Yeah, we also discussed tests. There, I'm satisfied with the use of (-) as a non-commutative and non-associative operation.

@josefs You could of course consider using the same or a similar function for the example too!

I've opened #264 to dicuss the fromListWith ergonomics.

sjakobi · 2020-06-06T10:38:09Z

@josefs I would like to include this PR in the upcoming release (see #266). Do you think you could address the outstanding documentation issue within the next 7 days?

@treeowl I assume you have vetted the internals and are happy with them!?

…ciative operators Since the combining function is applied in a way that can be counter-intuitive it's more pedagical to to use operators which better illustrate this behaviour.

josefs · 2020-06-06T18:11:51Z

@sjakobi I've updated the documentation to use division and subtraction instead to better illustrate the non-obvious way in which values are combined.

I really liked the free magma suggestion from @treeowl so I've converted the tests to use that.

I believe that should take care of all the outstanding issues for this PR so it should be good to go for the next release.

Btw, it might be good to squash at least some of these commits.

sjakobi

Thank you, @josefs! :)

josefs added a commit to josefs/unordered-containers that referenced this pull request Jan 20, 2020

Define unsafeInsertWith using unsafeInsertWithKey

8d09e9a

See haskell-unordered-containers#246 (comment) for performance numbers.

emilypi reviewed May 28, 2020

View reviewed changes

Data/HashMap/Strict/Base.hs Outdated Show resolved Hide resolved

sjakobi added the feature-request label May 31, 2020

sjakobi added this to the 0.2.11.0 milestone May 31, 2020

sjakobi requested changes Jun 2, 2020

View reviewed changes

josefs added 4 commits June 2, 2020 08:09

Add 'fromListWithKey' to HashMap

5fd4923

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Define unsafeInsertWith using unsafeInsertWithKey

821fe36

See haskell-unordered-containers#246 (comment) for performance numbers.

Improve documentation of fromListWithKey

2302fc6

Thanks to @emilypi for the suggestions

Add a test for fromListWithKey

4524df3

It models the test for fromListWith but makes sure that values are combined in a way that depends on the key.

josefs force-pushed the master branch from 8d09e9a to 4524df3 Compare June 2, 2020 15:14

sjakobi requested changes Jun 2, 2020

View reviewed changes

josefs added 3 commits June 2, 2020 08:55

Clarify the documentation for fromListWithKey

9e99f3c

Improve properties for fromListWith and fromListWithKey

b28a527

The old properties used associative operators to combine values when there were duplicate keys. With this diff we're using a non-commutative and non-associative operator which can catch more bugs.

Make sure to return an unboxed tuple in unsafeFromListWithKey

f945c63

This got lost in a rebase

Use the free magma to ensure that we preserve the order of operations

87e2b66

This was referenced Jun 2, 2020

fromListWith is confusing #264

Open

Prepare release 0.2.11.0 #266

Merged

Update fromListWithKey documentation to use non-commutative, non-asso…

506e0d4

…ciative operators Since the combining function is applied in a way that can be counter-intuitive it's more pedagical to to use operators which better illustrate this behaviour.

sjakobi approved these changes Jun 7, 2020

View reviewed changes

sjakobi merged commit 17127e0 into haskell-unordered-containers:master Jun 11, 2020

Add 'fromListWithKey' to HashMap #246

Add 'fromListWithKey' to HashMap #246

Uh oh!

Conversation

josefs commented Jan 17, 2020

Uh oh!

treeowl commented Jan 17, 2020

Uh oh!

josefs commented Jan 17, 2020

Uh oh!

josefs commented Jan 17, 2020

Uh oh!

josefs commented Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

josefs commented Jun 2, 2020

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjakobi Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

josefs Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

sjakobi Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

josefs Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

sjakobi Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

treeowl commented Jun 2, 2020

Uh oh!

treeowl commented Jun 2, 2020 via email

Uh oh!

sjakobi commented Jun 2, 2020

Uh oh!

sjakobi commented Jun 6, 2020

Uh oh!

josefs commented Jun 6, 2020

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josefs commented Jan 20, 2020 •

edited

Loading