Add mapMaybeM and imapMaybeM #226
Conversation
Add ```haskell mapMaybeM :: Monad m => (a -> m (Maybe b)) -> Vector a -> m (Vector b) imapMaybeM :: Monad m => (Int -> a -> m (Maybe b)) -> Vector a -> m (Vector b) ``` `mapMaybeM` is similar to `wither`, but the stream fusion framework seems to require that we use a `Monad` constraint rather than an `Applicative` one to get good performance. `imapMaybeM` is the indexed variant. Resolves haskell#183
There was a problem hiding this comment.
This looks like a fine addition to me! (I'm sort of surprised that we didn't have this already, in fact.)
I've left two suggestions inline, although you probably know this stuff better than I do, so answering "no" to my requests is a perfectly reasonable suggestion :)
| {-# INLINE mapMaybeM #-} | ||
| mapMaybeM f = unstreamM . Bundle.mapMaybeM f . stream | ||
|
|
||
| imapMaybeM :: (Monad m, Vector v a, Vector v b) |
There was a problem hiding this comment.
Is it worthwhile to mark imapMaybeM as INLINE? (Genuine question—I'm not sure if it would be beneficial here or not.)
There was a problem hiding this comment.
I sure hope that helps. @bgamari has pointed out some trouble fusing unstreamM, but we surely want to fuse on the stream side.
|
|
||
| imapMaybeM :: (Monad m, Vector v a, Vector v b) | ||
| => (Int -> a -> m (Maybe b)) -> v a -> m (v b) | ||
| imapMaybeM f = unstreamM . Bundle.mapMaybeM (\(i, a) -> f i a) . Bundle.indexed . stream |
There was a problem hiding this comment.
Other indexed functions in this module seem to use uncurry f instead of (\(i, a) -> f i a)—perhaps it would be worth sticking to this convention? (I'm not sure if using one or the other has any actual ramifications on strictness properties.)
There was a problem hiding this comment.
I never use uncurry because it introduces laziness I virtually never need. I bet the compiler often fixes that, but I see no reason to tempt fate.
There was a problem hiding this comment.
i wonder if the uncurry thing is actually necessary in general. i tend to prefer not using it as well.
|
Ooo. This looks like it’s a good idea. The wither style api pattern is a
great one. And it does sound like something that should play nice with
stream fusion.
I’ll need to go over this code very thoroughly but I’m very much in favor
of this addition
…On Tue, Nov 6, 2018 at 9:59 AM David Feuer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Data/Vector/Generic.hs
<#226 (comment)>:
> @@ -1334,6 +1334,14 @@ filterM :: (Monad m, Vector v a) => (a -> m Bool) -> v a -> m (v a)
{-# INLINE filterM #-}
filterM f = unstreamM . Bundle.filterM f . stream
+mapMaybeM :: (Monad m, Vector v a, Vector v b) => (a -> m (Maybe b)) -> v a -> m (v b)
+{-# INLINE mapMaybeM #-}
+mapMaybeM f = unstreamM . Bundle.mapMaybeM f . stream
+
+imapMaybeM :: (Monad m, Vector v a, Vector v b)
I sure hope that helps. @bgamari <https://github.com/bgamari> has pointed
out some trouble fusing unstreamM, but we surely want to fuse on the
stream side.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#226 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAQwnjyMU-MPGy5h7LGKmy5nuKub7X_ks5usaO7gaJpZM4YPZzs>
.
|
|
@cartazio, it actually shouldn't require terribly much review. The whole thing is an extremely gentle modification of |
|
FYI, the next release of |
|
Any updates? The change looks good |
|
It’s top of my holiday vacation list :)
…On Tue, Dec 17, 2019 at 10:25 PM Fumiaki Kinoshita ***@***.***> wrote:
Any updates? The change looks good
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#226?email_source=notifications&email_token=AAABBQRUSEIA6XEDLIEEVELQZGJ3NA5CNFSM4GB5TTWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEXHVQ#issuecomment-566850518>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQTMPUFEVPEZW4UNBTDQZGJ3NANCNFSM4GB5TTWA>
.
|
|
this is an often requested function, if we can add it to part of the test suite that would make me game for inclusion |
|
(inclusion for next release,) |
|
Adding documentation to those new functions would be a great idea. Otherwise LGTM |
|
ok, i'm game for merging this in as long as we gate the release on a contrib of docs and tests from whomever |
|
Only missing piece for this PR is documentation. I think easiest approach is to merge it as it is and I'll write haddock for functions later |
|
@Shimuuar if you'd rather not merge it into master without documentation, we could create a temporary branch from master for this PR and merge it into that branch, where you could finish up the doc. All that is necessary is creating a branch and changing the target of this PR |
|
I propose to merge it without documentation and it later separately. Seems like easiest approach |
|
Do I need to write documentation today? Doesn't sound like a big deal. |
|
No hurry. It's open for more than year. It could wait for few days |
|
@treeowl Could you please add haddocks and changelog entry? So this PR could be finally merged |
|
Superseded by #333 which is this PR rebased on top of master and added haddocks |
Add
mapMaybeMis similar towither, but the stream fusion frameworkseems to require that we use a
Monadconstraint rather than anApplicativeone to get good performance.imapMaybeMis theindexed variant.
Resolves #183