-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
foldMap, foldMap' for PrimArray #186
base: master
Are you sure you want to change the base?
Conversation
I'm good with this. If there aren't any objection in the next few days, I'll merge this. |
No objections here, although these deserve to be mentioned in the changelog. |
Sorry I keep forgetting that. I'll add it in. |
Is this good to be merged? The travis failure is unrelated. |
Data/Primitive/PrimArray.hs
Outdated
foldMapPrimArray f = foldrPrimArray (\a acc -> acc `mappend` f a) mempty | ||
|
||
-- | Strictly map each element of the primitive array to a monoid, and combine the results. | ||
foldMapPrimArray' :: forall a m. (Prim a, Monoid m) => (a -> m) -> PrimArray a -> m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is insufficient. It should explain which way the combination associates. Also, it's notably not (necessarily) strict in the function results. The documentation should reflect this and there should be a comment explaining that decision.
Data/Primitive/PrimArray.hs
Outdated
@@ -467,6 +469,16 @@ sizeofPrimArray :: forall a. Prim a => PrimArray a -> Int | |||
{-# INLINE sizeofPrimArray #-} | |||
sizeofPrimArray (PrimArray arr#) = I# (quotInt# (sizeofByteArray# arr#) (sizeOf# (undefined :: a))) | |||
|
|||
-- | Lazily map each element of the primitive array to a monoid, and combine the results. | |||
foldMapPrimArray :: forall a m. (Prim a, Monoid m) => (a -> m) -> PrimArray a -> m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is insufficient. It should explain how the combination associates. In fact, I think we should almost surely offer more than one version: associating to the left, associating to the right, and (for certain tree-like monoids) associating in a balanced fashion are all potentially useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tree-like or in some cases numerical. Balanced association can reduce floating point errors somewhat, for example.
Sorry for the delayed review |
That is okay, this is valuable feedback. I will make these changes shortly. |
most recent commit does not add balanced foldMap. I think I'd prefer for that to be in a separate PR, as it's a little more complicated. |
Data/Primitive/PrimArray.hs
Outdated
@@ -467,6 +471,32 @@ sizeofPrimArray :: forall a. Prim a => PrimArray a -> Int | |||
{-# INLINE sizeofPrimArray #-} | |||
sizeofPrimArray (PrimArray arr#) = I# (quotInt# (sizeofByteArray# arr#) (sizeOf# (undefined :: a))) | |||
|
|||
-- | Map each element of the primitive array to a monoid, and combine the results. | |||
-- The combination is right-associated, and the accumulation is lazy. | |||
foldrMapPrimArray :: forall a m. (Prim a, Monoid m) => (a -> m) -> PrimArray a -> m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather see the Map
before the r
.
Data/Primitive/PrimArray.hs
Outdated
|
||
-- | Map each element of the primitive array to a monoid, and combine the results. | ||
-- The combination is right-associated, and the accumulation is strict, though | ||
-- this function is not necessarily strict in its result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This function is not necessarily strict in its result" doesn't mean anything whatsoever to me. What I meant needed explanation is that at each step, we force the accumulator value, but we don't force the value of f a
. So if mappend
is lazy in its first argument, f a
will not be evaluated. This is probably the correct behavior, but needs better explanation.
changelog.md
Outdated
@@ -29,7 +29,9 @@ | |||
|
|||
## Changes in version 0.6.4.1 | |||
|
|||
* Add instances for the following newtypes from `base`: | |||
* Add `foldMapPrimArray` and `foldMapPrimArray'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated now that you've added more functions.
…ct some documentation on strict folds
requested changes attempted, @treeowl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. Just a couple more little changes.
Both examples are supposed to show associativity. I think they can be much
briefer, TBH. Just give a three-element example application and show how it
unfolds as a single expression.
…On Mon, Sep 17, 2018, 9:54 PM chessai ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Data/Primitive/PrimArray.hs
<#186 (comment)>:
> +--
+-- @mysum = 'Data.Monoid.getSum' '$' ('Data.Monoid.Sum' 1 '<>' ('Data.Monoid.Sum' 2 '<>' ('Data.Monoid.Sum' 3 '<>' 'Data.Monoid.Sum' 4)))@
+--
+-- @mysum = 'Data.Monoid.getSum' '$' ('Data.Monoid.Sum' 1 '<>' ('Data.Monoid.Sum' 2 '<>' ('Data.Monoid.Sum' 7)))@
+--
+-- @mysum = 'Data.Monoid.getSum' '$' ('Data.Monoid.Sum' 1 '<>' ('Data.Monoid.Sum' 9)@
+--
+-- @mysum = 'Data.Monoid.getSum' '$' 'Data.Monoid.Sum' 10@
+--
+-- @mysum = 10@
+foldMapRPrimArray :: forall a m. (Prim a, Monoid m) => (a -> m) -> PrimArray a -> m
+{-# INLINE foldMapRPrimArray #-}
+foldMapRPrimArray f = foldrPrimArray (\a acc -> f a `mappend` acc) mempty
+
+-- | Map each element of the primitive array to a monoid, and combine the results.
+-- The combination is left-associated, and the accumulation is lazy.
Clarification: Are you asking to provide an example for foldMapLPrimArray,
to show left-associativity (for this particular lazy fold)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#186 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_V_eja4orM-34lJJs1TwInJE6ERrks5ucFJVgaJpZM4VQDt8>
.
|
By 'briefer', do you mean less 'tutorial-like', to assume that the example is illustrative enough sans written explanation? |
Right. This is |
Makes sense - understood |
Aha! I've changed my mind :). I believe we should force each function value
in the strict versions. Laziness here is kind of silly, but more to the
point, I believe the laziness reveals semantic differences in fold
associativity that are only supposed to be performance differences.
…On Mon, Sep 17, 2018, 10:17 PM chessai ***@***.***> wrote:
Right. This is primitive, not base or containers. Users are expected to
know what they're doing. But language is slippery, so "associates to the
left" might well evoke opposite meanings in the minds of two different
people.
Makes sense - understood
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#186 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_a3CzXzv1s-TcI2ZeePIIZX1mptbks5ucFe5gaJpZM4VQDt8>
.
|
@treeowl Examples are now simplified and function values are being forced |
Humor me and check whether what I said about forcing is really true. I was
going off intuition.
…On Mon, Sep 17, 2018, 10:53 PM chessai ***@***.***> wrote:
@treeowl <https://github.com/treeowl> Examples are now simplified and
function values are being forced
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#186 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_VlFO4Cj6buIYDs2COP8-OJAPoAPks5ucGASgaJpZM4VQDt8>
.
|
OK |
I like the stuff in the PR, although I dislike the naming convention that @treeowl suggested. I actually prefer |
I would still like this and also a foldl for |
I'll change the names and merge this. @Boarders Open a separate PR for a foldl on |
No description provided.