-
Notifications
You must be signed in to change notification settings - Fork 2
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
GenIO #15
Comments
Two separate PRs implement both concepts:
Here are the benchmarks with Let's look at pros and cons for both of these: Both have the same pro that they can be used in 1 -
|
I still don't understand why anyone would want to do
rather than
Looking at your performance chart above, there seems to be no performance advantage so what am I missing? |
This is necesarry for example for
|
I think there's case for adding type class along the lines:
People will want to use such API and
|
@Shimuuar I disagree. What you are suggesting is an alternative approach to what we already have. I think it is a very limiting design. What you are suggesting is essentially the same thing as what we have now, but with fun deps:
This reduces number of generators it can be used with, and that is exactly why I suggest we adjust To sum it up, we do not need such type class.
|
Both designs are equal. Any instance for So I don't think that one design is strictly better than another. They make different tradeoffs. If you writing some Monte-Carlo code carrying same Also what @idontgetoutmuch think about this? |
@Shimuuar They are not equal, because this class cannot be used with stateful generators:
You cannot have these two instances at the same time: import System.Random.MWC as MWC
instance MonadRandom IO where
type PRNG IO = MWC.Gen RealWorld import System.Random.PCG as PCG
instance MonadRandom IO where
type PRNG IO = PCG.Gen RealWorld |
They absolutely can. But one have to add newtype wrappers: instance MonadRandom (ReaderT (MWC.Gen RealWorld) IO) where
type PRNG (ReaderT (MWC.Gen RealWorld) IO) = MWC.Gen RealWorld This solution is of course far satisfactory but it illustrates idea. This is why I said this style forces use of newtypes. |
I am trying to tell you that this idea is flawed! It doesn't solve the problem I am talking about!
But that will conflict with
@Shimuuar I guess, instead of throwing ideas left and right the best way for you to demonstrate the concept is to implement it and submit a PR. |
instance MonadRandom (ReaderT (PCG.Gen RealWorld) IO) where
instance MonadRandom (ReaderT (MWC.Gen RealWorld) IO) where Instance heads are different so there's no overlap
In more realistic example I'd create newtype wrapper around ReaderT and would define instance around it. ReaderT and other transformer's monads would get passthrough instance (mtl curse)
I do argue a lot but here I agree with you :) |
@Shimuuar I think I have an elegant solution for your concerns. I am using The gist of it is: Definition of a class HasGen env g | env -> g where
genL :: Lens' env g
instance RandomGen g => HasGen (PureGen g) (PureGen g) where
genL = lens (const PureGenI) const
instance RandomGen g => HasGen (PrimGen s g) (PrimGen s g) where
genL = id Definition of helper functions that are reader and random aware: uniformEnv :: (MonadReader env m, MonadRandom g m, HasGen env g, Uniform a) => m a
uniformEnv = do
env <- ask
uniform (env ^. genL) Usage of such functions becomes very easy and doesn't require generator to be passed around manually: foo :: (MonadReader env m, MonadRandom g m, HasGen env g) => m Int
foo = do
x :: Int <- uniformEnv
y :: Int <- uniformEnv
pure (x + y) Then you get exactly what you where asking for: runPureGenState :: RandomGen g => g -> ReaderT (PureGen g) (State g) a -> (a, g)
runPureGenState g = runGenState g . flip runReaderT PureGenI
bazState :: StdGen -> (Int, StdGen)
bazState g = runPureGenState g foo But most importantly it allows you to keep mutable generators (such as in mwc-random for example) in the reader environment: data MyEnvPrimGen = MyEnvPrimGen {
myEnvPrimGen :: PrimGen RealWorld StdGen
}
instance HasGen MyEnvPrimGen (PrimGen RealWorld StdGen) where
genL = lens myEnvPrimGen (\e g -> e { myEnvPrimGen = g })
bazPrim :: StdGen -> IO (Int, StdGen)
bazPrim g = do
(res, PrimGen g') <-
withGenM (PrimGen g) $ \mg -> flip runReaderT (MyEnvPrimGen mg) foo
pure (res, g') @Shimuuar Let me know what you think or if you have questions about this approach. We use lens+reader extensively in rio and it works really well. |
@Shimuuar doesn't this already exist in
|
I agree we should try and simplify |
My usual pattern for random numbers is to use
I certainly wouldn't want to thread a generator through all my code but I don't have to with EDIT: Maybe this is a better example of usage.
@Shimuuar: does that help or were you asking me a different question? |
I think found another design which avoids duplication of API in |
@Shimuuar did you get a chance to look at what I wrote? Using Reader solves the problem of need to carry the generator around, but it doesn't force that pattern on the user. |
I did. I however won't comment on it until I prepate my PR so it's possible
to compare both approaches
|
@Shimuuar did you ever prepare a PR? I think we have made quite a lot of progress now and would be very interested in your comments / proposal. |
Sadly no. I had no time to work at random at all for last two weeks |
@Shimuuar are you still looking into this? If not, I'd vote to close this issue. |
Yes. I think this could be closed |
👍 |
Current interface has a way of generating values in IO with
randomIO
. @Shimuuar pointed out that it could be useful to provide interface for such generator.There are a couple of choices we can make here. We could just as before wrap pure
StdGen
in anIORef
, but a better choice would be to usePrimMonad
GenPrim
Creating a more general
GenPrim s
by wrappingStdGen
in aMutVar
that can be used inST
andIO
as well as all of the transformersAtomicity
Splitmix uses two
Word64
s for its state, so if we want atomicity the only right way is theMutVar
, on the other hand if atomicity is not a requirement then we could use a smallMutableByteArray
for it's state therefore speeding up value generation in the stateful environment by avoiding one pointer indirection.I personally propose having both of those implemented, since both of the implementations are pretty easy and straightforward. The latter faster-nonatmoic version is curently blocked by an actual switch to splitmix generator, however an even more general interface could be prepared for all pure nonatomic generators that have instance of
Prim
and/orStorable
The text was updated successfully, but these errors were encountered: