Use RandomGen in sampleState and sampleStateT

[FintanH]

Fixes #44

• Export RandomGen in Data.Random.Source
• Use RandomGen as a type class constraint in sampleState and sampleStateT
• Removed whitespace (shows up in my vim so quickly removed it :smile:)

[FintanH]

Ah it seems I've done something wrong here. It's not actually building properly because it's not picking up my local changes in random-source but instead uses the hackage version. I might just opt to import RandomGen in random-fu so.

[idontgetoutmuch]

Can you explain why this is better than e.g.

$(randomSource [d|
  instance (PrimMonad m) => RandomSource (ReaderT TF.TFGen m) TF.TFGen where
    getRandomWord64From = undefined
    getRandomDoubleFrom = undefined
  |])

which compiles but what should the undefined be? mwc-random provides uniform so we can replace undefined with uniform and fmap wordToDouble . uniform. Maybe you can write uniform for tf-random?

[FintanH]

Thanks for taking a look at this and the related issue. Sorry for coming in guns blazing but I wanted to test this in some related work code :) It took me a bit to understand the inner workings, I'll blame it on trying to code too late on a Friday evening :grimacing:

So my thinking around this is that RandomGen is the class for random generators and since things like StdGen, PureMT, and TFGen are all instances of the class we could utilise this information to make a more general implementation that doesn't lock users into making instances of MonadRandom (State s). In other words, allow the library to be extensible in terms of the types of seeds they can use.

What do you think? Otherwise I can take a crack at adding RandomSource (ReaderT TFGen m) TFGen.

[idontgetoutmuch]

I just tried building your PR.

[12 of 12] Compiling Data.Random.Source.StdGen ( src/Data/Random/Source/StdGen.hs, .stack-work/dist/x86_64-osx/Cabal-2.4.0.1/build/Data/Random/Source/StdGen.o )

src/Data/Random/Source/StdGen.hs:61:1: warning:
    You cannot SPECIALISE ‘RandomGen.getRandomPrimFromRandomGenState’
      because its definition has no INLINE/INLINABLE pragma
      (or its defining module ‘Data.Random.Source.RandomGen’
       was compiled without -O)
   |
61 | {-# SPECIALIZE RandomGen.getRandomPrimFromRandomGenState :: Prim a -> State StdGen a #-}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

src/Data/Random/Source/StdGen.hs:62:1: warning:
    You cannot SPECIALISE ‘RandomGen.getRandomPrimFromRandomGenState’
      because its definition has no INLINE/INLINABLE pragma
      (or its defining module ‘Data.Random.Source.RandomGen’
       was compiled without -O)
   |
62 | {-# SPECIALIZE RandomGen.getRandomPrimFromRandomGenState :: Monad m => Prim a -> StateT StdGen m a #-}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Installing library in /Users/dom/random-fu/.stack-work/install/x86_64-osx/lts-13.7/8.6.3/lib/x86_64-osx-ghc-8.6.3/random-source-0.3.0.7-FusTRsjNzNV9M7UsMiVDlI
Registering library for random-source-0.3.0.7..

More worryingly I get an error when building speed-tests

Bench.hs:43:38: error:
    • Overlapping instances for RandomSource
                                  IO (GHC.IORef.IORef System.Random.Mersenne.Pure64.Internal.PureMT)
        arising from a use of ‘dists’
      Matching instances:
        two instances involving out-of-scope types
        (use -fprint-potential-instances to see them all)
    • In the second argument of ‘bgroup’, namely ‘(dists mtSrc count)’
      In the expression: bgroup "PureMT" (dists mtSrc count)
      In the second argument of ‘bgroup’, namely
        ‘[bgroup "MWC" (dists mwcSrc count),
          bgroup "PureMT" (dists mtSrc count),
          bgroup "StdGen" (dists stdSrc count),
          bgroup "DevRandom" (dists DevRandom count), ....]’
   |
43 |             , bgroup "PureMT"       (dists mtSrc        count)
   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^

[idontgetoutmuch]

Here's a bit more information

*Main> :set -fprint-potential-instances
*Main> :r
[36 of 36] Compiling Main             ( tests/speed/Bench.hs, interpreted )

tests/speed/Bench.hs:43:38: error:
    • Overlapping instances for RandomSource
                                  IO (GHC.IORef.IORef System.Random.Mersenne.Pure64.Internal.PureMT)
        arising from a use of ‘dists’
      Matching instances:
        two instances involving out-of-scope types
          instance (Monad m, RandomGen g,
                    Data.StateRef.Types.ModifyRef (GHC.IORef.IORef g) m g) =>
                   RandomSource m (GHC.IORef.IORef g)
            -- Defined at /Users/dom/random-fu/random-source/src/Data/Random/Source/RandomGen.hs:34:10
          instance MonadIO m =>
                   RandomSource
                     m (GHC.IORef.IORef System.Random.Mersenne.Pure64.Internal.PureMT)
            -- Defined at /Users/dom/random-fu/random-source/src/Data/Random/Source/PureMT.hs:107:3
    • In the second argument of ‘bgroup’, namely ‘(dists mtSrc count)’
      In the expression: bgroup "PureMT" (dists mtSrc count)
      In the second argument of ‘bgroup’, namely
        ‘[bgroup "MWC" (dists mwcSrc count),
          bgroup "PureMT" (dists mtSrc count),
          bgroup "StdGen" (dists stdSrc count),
          bgroup "DevRandom" (dists DevRandom count), ....]’
   |
43 |             , bgroup "PureMT"       (dists mtSrc        count)
   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^

[idontgetoutmuch]

Ok I think I am with you now. Rather than e.g. writing

...
$(monadRandom
    [d| instance MonadRandom (State TFGen) where
            getRandomWord64 = withMTState randomWord64
            getRandomDouble = withMTState randomDouble
     |])

$(monadRandom
    [d| instance MonadRandom (S.State TFGen) where
            getRandomWord64 = withMTState randomWord64
            getRandomDouble = withMTState randomDouble
     |])
...

with

randomWord64 :: TFGen -> (Word64, TFGen)
randomWord64 = random

randomDouble :: TFGen -> (Double, TFGen)
randomDouble = undefined

you define such an instance for anything in RandomGen.

But what should we do about overlapping instances? We could just remove the offending instance from PureMT.hs:

$(randomSource
    [d| instance (MonadIO m) => RandomSource m (IORef PureMT) where
            getRandomWord64From = withMTRef randomWord64
            getRandomDoubleFrom = withMTRef randomDouble
     |])

But your change seems to be a breaking change since folks could have created their own instances? Perhaps this should be random-source-0.4.0?

Would it be possible to create a CHANGELOG.md with a note indicating what the change is, why it is a breaking change and what the library user should do on encountering the breakage?

I note that we have no CI nor any great tests but that's nothing to do with your PR. Again many thanks for taking the time.

[idontgetoutmuch]

Another two things (sorry): will this affect performance? Possibly for the duplicate instance of MT only. What about the "cannot specialise" warnings?

[idontgetoutmuch]

I have now set up CI (https://circleci.com/gh/haskell-numerics/random-fu) and in doing so decided to put the main repo here: https://github.com/haskell-numerics. I'd like to make a new release with your PR in it if that's possible.

[FintanH]

Hey @idontgetoutmuch sorry been doing some travelling, but going to take a look at this now.

1) The overlapping instance is because of PureMT being an instance of RandomGen 2) I think we can mark the instance as INLINABLE which tends to perform better than SPECIALISE. 3) Sorry about the messy compilation. I didn't have a good set up for this :(

[idontgetoutmuch]

@FintanH I had to revert this as it causes ghc to panic: https://gitlab.haskell.org/ghc/ghc/issues/18118

[FintanH]

oof, it's been a while since I wrote this. If you need help with it again let me know :) I can't even remember what I wanted it for now :sweat_smile:

[idontgetoutmuch]

No worries - I wanted to let you know your efforts were not in vain - I am trying to fix the problem which is almost certainly something to do with ghc or nix.

[FintanH]

Thanks, I appreciate it :)

[idontgetoutmuch]

@FintanH FYI - it caused a performance regression so sadly I have reverted it - thanks for contributing.