-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add generate, modifyM & folds for mutable vectors #338
Conversation
I have no issues with foldl :: (PrimMonad m, MVector v a) => (b -> a -> b) -> b -> v (PrimState m) a -> m b But I'm unsure about monadic folds like foldM :: (PrimMonad m, MVector v a) => (b -> a -> m b) -> b -> v (PrimState m) a -> m b Do we really need them? What useful monadic effect could happen in |
@Bodigrim You should not feel any danger about it, because there nothing unsafe about monadic folds. On the other hand my opinion about foldl :: (PrimMonad m, MVector v a) => (b -> a -> b) -> b -> v (PrimState m) a -> m b
foldl f acc vec = .. It gives you this feeling that the function So I would recommend is adding all four: foldlM :: (PrimMonad m, MVector v a) => (b -> a -> m b) -> b -> v (PrimState m) a -> m b
ifoldlM :: (PrimMonad m, MVector v a) => (b -> Int -> a -> m b) -> b -> v (PrimState m) a -> m b
foldrM :: (PrimMonad m, MVector v a) => (a -> b -> m b) -> b -> v (PrimState m) a -> m b
ifoldrM :: (PrimMonad m, MVector v a) => (Int -> a -> b -> m b) -> b -> v (PrimState m) a -> m b |
I don't think possibility that callback function will modify vector is a problem. As for pure folds. I think about them as syntactic convenience. To avoid wrapping pure function into monads if it happens to be pure. Compare: foldl (+) 0 vec
foldM (\x y -> pure (x +y)) 0 vec Also I don't see how they could be interpreted as pure. Result is wrapped in monad after all. And vectors are being modified concurrently all bets are off in any case. P.S. Agree about right folds |
100% I am not against adding folds with pure functions. They definitely can't be interpreted as pure, so it is no surprise you don't see it ;) . All I am saying is that they can be mistakenly interpreted as pure by a novice user, because by itself a |
-- Folds | ||
-- ----- | ||
|
||
forI_ :: (Monad m, MVector v a) => v (PrimState m) a -> (Int -> m b) -> 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.
Shouldn't it be PrimMonad 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.
Not really. This function doesn't use anything from PrimMonad, Callback that passed as parameter usually does
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.
Isn't PrimMonad m
required to use PrimState m
? PrimState
is defined inside the PrimMonad
class, so I thought that constraint would be required to be able to use it.
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.
Sorry for late answer. Not quite. It's true that in order to define PrimState
type family instance one have to define PrimMonad
instance. But when it comes to use associated type work just like stand alone type families. Since we don't use any methods of PrimMonad
we don't need to bring its dictionary here.
This sounds great. Will you add right folds in this PR too or postpone it to a later one? |
I'll add them here. Thanks for reminding about them |
If you're also adding right fold (I assume including monadic right folds), wouldn't it make more sense to use |
@konsumlamm Thanks! |
Only unresolved point is naming: |
Perhaps we could do both and make |
I actually was gonna suggest the same thing of naming I am also not in favor of adding |
- mapM_, imapM_ - foldl', fold and strict & indexed variants
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
I've added reexports for (hopefully) everything. Also as it turns out we didn't have reexports for |
@konsumlamm thanks, for noticing this
And fix API discrepancies for left folds
Not ready for merge. I started writing tests and found out that I used wrong order of parameters for indexed folds. And now GHC produces tests suite that crashes. I'll start investigating this tomorrow |
Note test suite crashes for me at this commit
And fix segfault
Also fix case for negative vector size
Turns out writing tests is pretty neat idea. They caught both discrepancies in API between pure and mutable versions and violations of memory safety. Now I think PR is ready for review. |
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.
Just a few minor suggestions. Haven't found any correctness issues, so it looks great.
-- and fill it with the results of applying the function to each index. | ||
generate :: (PrimMonad m, MVector v a) => Int -> (Int -> a) -> m (v (PrimState m) a) | ||
{-# INLINE generate #-} | ||
generate n f = stToPrim $ generateM n (return . f) |
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.
Does stToPrim
help the optimizer or improves performance somehow? It seems redundant to me
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.
It allows GHC to work in the ST monad instead of whatever tower of monad transformers is thrown at it. GHC may or may not be able to optimize those binds. Hopefully this will reduce amount of work optimizer has to do and may even result in better code. Same for other function that take pure function as parameters
Data/Vector/Generic/Mutable.hs
Outdated
-- | /O(n)/ Pure left fold (function applied to each element and its index). | ||
ifoldl :: (PrimMonad m, MVector v a) => (b -> Int -> a -> b) -> b -> v (PrimState m) a -> m b | ||
{-# INLINE ifoldl #-} | ||
ifoldl f b0 v = stToPrim $ loop 0 b0 |
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.
Here again and in all other folds, it doesn't look like we need stToPrim
. unsafeRead
is restricted to PrimMonad
, not ST
.
Also as with other suggestion we can use ifoldM
to define ifoldl
prop_mut_foldrM' :: P ((a -> a -> Identity a) -> a -> v a -> Identity a) | ||
= (\f z v -> Identity $ runST $ MV.foldrM' (\a b -> pure $ runIdentity $ f a b) z =<< V.thaw v) | ||
`eq` | ||
foldrM |
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.
Hmm, look like we don't have a corresponding pure strict right monadic fold: foldrM'
for immutable vectors.
Just mentioning this in case you are in a mood of adding it for completeness. Feel free to ignore this comment if you don't feel like adding it in this PR.
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.
We don't have lazy foldrM
either. I'll add them in separate PR. This PR is large enough as it is
Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
@lehins I finally addressed your suggestions. I also added changelog |
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
@Shimuuar This looks great. Changes are ready to be merged, but it would be nice to cleanup the commit history. If you aren't up to it I can just squash all of the changes into one and merge it. Once it is merged I'll backport changes in this PR to vector-0.12.3.0 since we need to make another non-breaking release anyways that includes #372 |
Let just squash it. it not that big and changes are sometimes interwoven and couldn't be cleaned up easily |
Add generate, modifyM & folds for mutable vectors: * Added `generate`, `generateM` for mutable vectors * Added `modifyM` and `unsafeModifyM` for mutable vectors * Add all variants of folds for mutable vectors: * `mapM_`, `imapM_`, `forM_`, `iforM_` * `foldl`, `foldl'`, `foldM`, `foldM'`, * `foldr, `foldr'`, `foldrM`, `foldrM'`, * `ifoldl`, `ifoldl'`, `ifoldM`, `ifoldM'`, * `ifoldr`, `ifoldr'`, `ifoldrM, `ifoldrM'` * Add tests for all new functions * Update changelog
See #334 and linked issues for discussion. Draft since functions were only added to generic module
These I think completely uncontroversial function