Add 'compose' for maps #672
Conversation
| -- @since UNRELEASED | ||
| compose :: IntMap c -> IntMap Int -> IntMap c | ||
| compose bc ab = if null bc | ||
| then empty |
There was a problem hiding this comment.
This is surprisingly lazy in the second argument. Is that special case a good special case? I'm not so convinced yet. Please make an argument.
There was a problem hiding this comment.
I don't think I yet sufficiently grasp the laziness expectations in these modules to answer properly. Here's what I managed to gather after reading your question:
The strict variants say
If all values stored in all maps in the arguments are in WHNF, then all values stored in all maps in the results will be in WHNF once those maps are evaluated.
Does the same apply to keys for lazy maps given we have
A Map is strict in its keys but lazy in its values.
, and in that case ab should have its keys (and values, if strict) always evaluated if the result map is evaluated? Thus making optimizations like this one here somewhat break the interface?
There was a problem hiding this comment.
If the first map is empty, then we don't force the second map. This seems a little weird, so maybe we should just write compose bc !ab | null bc = ..... For style, we should use guards rather than if/then here.
There was a problem hiding this comment.
Oh I see, the second argument is potentially lazier than the first
| {-------------------------------------------------------------------- | ||
| Compose | ||
| --------------------------------------------------------------------} | ||
| -- | /O(|ab|*log(|bc|))/. Relate the keys of one map to the values of |
There was a problem hiding this comment.
That bound looks wrong. Poke around the rest of the module to see what the bounds look like.
There was a problem hiding this comment.
Do you mean that it should use n & m like the other functions? I strayed from that because other bounds seem to be commutative in their arguments, so they don't need to say which argument map is n and which is m, but compose is asymmetric
There was a problem hiding this comment.
No, I mean that IntMap performance doesn't tend to involve logarithms of the size; instead, it usually relies on the word size. This is because the depth of the tree is not bounded by the logarithm of the total size. Check the bounds for (!?), which you use here.
There was a problem hiding this comment.
Ah yes, makes sense
| compose bc ab = if null bc | ||
| then empty | ||
| else mapMaybe (bc !?) ab | ||
|
|
There was a problem hiding this comment.
Same correction and comments apply here.
| {-------------------------------------------------------------------- | ||
| Compose | ||
| --------------------------------------------------------------------} | ||
| -- | /O(|ab|*log(|bc|))/. Relate the keys of one map to the values of |
There was a problem hiding this comment.
This bound is a bit funny when |bc| ≤ 1. Is it worth fixing? Not sure.
There was a problem hiding this comment.
Wouldn't that also apply to the existing O(n log n) bounds?
Big O notation is asymptotic, so in a sense there's nothing to fix.
Is there something specific about this bound you intend to make clearer? e.g. the if null bc case
|
After doing a few rounds of tweaks for all 4 implementations, given that they all kind of have the same implementation, I wonder if it would simplify maintenance to have a neutral/generic compose: compose empty' lookup' bc !ab
| null bc = empty'
| otherwise = mapMaybe (flip lookup' bc) abso that the actual implementations would be something like compose = Utils.compose empty lookup |
|
@treeowl I think I addressed the remarks - is there anything else I should do for this PR? |
|
I'll try to have another look tonight. I expect this to go in before the next release. |
|
Would you please rebase? I will then merge. Sorry it's been a bit. |
|
Thanks a lot! |
Implement #647 based on community input in mailing list.
I see most functions don't have an entry in the benchmarks. Should I add anything with this PR?
cc @Ericson2314