Skip to content
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

Refactor to reflect structure of mtl and transformers libraries #25

Merged
merged 4 commits into from
Nov 1, 2016

Conversation

markborkum
Copy link
Contributor

For more information, see the file CHANGES.markdown

Closes #23

For more information, see the file CHANGES.markdown

Closes byorgey#23
Stability : experimental
Portability : non-portable (multi-param classes, functional dependencies, undecidable instances)

Lazy random monads, passing a random number generator through a computation.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this say 'strict'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should.

I've submitted a patch (see: 8648613acb6a90d29c154387e7faf3605447e68a).

@byorgey
Copy link
Owner

byorgey commented Aug 3, 2016

Thanks for this! I'm still trying to wrap my head around all the changes. A few questions:

  • Can you give an example of the difference between a lazy and strict random monad?
  • Why remove evalRandIO?

@markborkum
Copy link
Contributor Author

No problem at all! Happy to contribute. Especially to a library that I use in pretty much all of my Haskell projects.

Lazy and strict variants are included for two reasons:

  1. Completeness - RandT is defined in terms of StateT, for which lazy and strict variants are provided. To me, having both variants is a no-brainer (the classic "because why not?!" rationale).
  2. Non-IO-based random number generators - Having a strict RandT type could support new use cases, where end-users have custom instances of the RandomGen class that, perhaps, do not depend on IO.

I removed the evalRandIO function for four reasons:

  1. Triviality - It is readily defined in terms of other functions.
  2. (Lack of) Generality - End-users may want to use getStdGen instead of newStdGen. Or, perhaps, not use StdGen at all.
  3. Generalization - It is not clear how to generalize the function for arbitrary Monads. For example (see below), should obtaining the random number generator be lifted to MonadIO?
  4. Optimization - Implementing the function in-place makes the code more visible to GHC's optimization algorithms.
evalRandIO :: Rand StdGen a -> IO a
evalRandIO x = fmap (evalRand x) newStdGen

evalRandTIO :: (MonadIO m) => RandT StdGen m a -> m a
evalRandTIO x = liftIO newStdGen >>= evalRandT x

@byorgey
Copy link
Owner

byorgey commented Aug 3, 2016

Triviality - It is readily defined in terms of other functions.

Sure, but in some sense the same argument applies to this entire library, which is easily defined in terms of StateT. There is still a nontrivial cost for users to figure out how to put together pieces to build what they want, and providing some convenience functions for common use cases is always nice, with the added benefit that users can learn from their implementation.

(Lack of) Generality - End-users may want to use getStdGen instead of newStdGen. Or, perhaps, not use StdGen at all.

Sure, but that is not an argument for removing evalRandIO. Users are not forced to use it. They can always do these other things whether it exists or not.

Generalization - It is not clear how to generalize the function for arbitrary Monads. For example (see below), should obtaining the random number generator be lifted to MonadIO?

Again, this may be true but it does not seem like an argument for removing it.

Optimization - Implementing the function in-place makes the code more visible to GHC's optimization algorithms.

This seems dubious to me. If we are really so worried about optimization we could add an INLINE pragma.

In short, I don't think these are good reasons, and I am strongly against removing this function, which will probably break existing user code. I personally use evalRandIO quite a bit for simple one-off things (e.g. if I am playing around in GHCi and need some random numbers).

@markborkum
Copy link
Contributor Author

In short, I don't think these are good reasons, and I am strongly against removing this function, which will probably break existing user code.

It is a breaking change. That's for sure. GitHub search finds 449 uses of evalRandIO.

I personally use evalRandIO quite a bit for simple one-off things (e.g. if I am playing around in GHCi and need some random numbers).

This is a very good point.

If evalRandIO were retained, would you be opposed to including evalRandTIO?

@byorgey
Copy link
Owner

byorgey commented Aug 3, 2016

If evalRandIO were retained, would you be opposed to including evalRandTIO?

Not at all.

@byorgey
Copy link
Owner

byorgey commented Aug 3, 2016

GitHub search finds 449 uses of evalRandIO.

Right. In that case, even if we were to remove it (which I still see no reason to do) we would at the very least want to deprecate it first and remove it in a subsequence release.

@@ -1,3 +1,9 @@
0.5.1 (3 Aug 2016)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be folded into the 0.5 changes below, I am not planning to make two separate 0.5 and 0.5.1 releases.

@byorgey
Copy link
Owner

byorgey commented Nov 1, 2016

Sorry it's taken me so long to get around to this. There are a bunch of pending changes to the library, and I finally have the time to do a proper job of incorporating everything and making a new release.

@byorgey byorgey merged commit 5b36cb1 into byorgey:master Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants