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

Update Polysemy.Random to use the newer System.Random "Uniform" and "UniformRange" interfaces. #81

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ShapeOfMatter
Copy link

It's awkward using the existing Polysemy.Random, because it maps to the Random class, which the current documentation suggests shouldn't be used in new code.

This pull request fixes that, and adds relevant unit tests and QOL details.

@ShapeOfMatter
Copy link
Author

Note that there are a few other ways these changes could have been handled:

  • Changing the signatures of the existing effect-constructors is plausibly a breaking change for someone. A different option would have been to expand the API to include random, randomR, and uniform and uniformR. I don't like that because it adds cruft/overhead to all interpretations (and it recapitulates more details from System.Random, which is contrary to the spirit of Polysemy).
  • The provided helper functions have types that are as open as possible but also enforce each function's needs as much as possible. We could be less fancy, at the expense of adding more warnings to the docs (and/or removing options).
  • I'd have liked to have a helper infiniteSamples :: Sem r (Data.Stream.Stream a), but it wasn't clear to me today how to write it without adding something analogous to System.Random.split to the API, and I wasn't sure we'd want that extra complexity.

@tek
Copy link
Member

tek commented Jul 11, 2023

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

@ShapeOfMatter
Copy link
Author

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

Interesting... At first I didn't like it but I'm opening to the idea. Where's the variant you use?

  • It'd be a big change; who-all would we want buy-in from?
  • Could the effect type parameter be a "sufficient class" (:: * -> Constraint) instead of a specific type? I'm unsure if that would be better or not.
  • It seems like we'd need to split random and randomR into separate effects?

Anyway, I think it's a bigger change than I'm proposing in this PR. If other people are enthusiastic about it I can participate in the work, starting with opening a new Issue...

@tek
Copy link
Member

tek commented Jul 15, 2023

It's pretty banal: https://github.com/tek/polysemy-hasql/blob/main/packages/db/lib/Polysemy/Db/Interpreter/Random.hs

anyway, I just wanted to ask your opinion, you can proceed with this without changing the effect further. I haven't used it enough to have an informed view on the matter.

so, PR looks fine to me!

@ShapeOfMatter
Copy link
Author

ShapeOfMatter commented Jul 18, 2023

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

Thinking about it more, I don't think this suggestion is the way forward.

  • If the effect is data Random x m a where random :: Random x m x, then isn't it just an alias for Input? I appreciate the desire to have each thing you want on hand with a relevant name, but I really think there'd be no value to it. Specifically, I think there'd still be demand for something more like the existing Random with its randomR function.
  • If the effect is data Random c m a where random :: (c x) => Random c m x, that would open up a lot of flexibility! But it still couldn't handle both random and randomR.
  • Is there a practical way to encode both the constraint and an implied argument type? Something like data Random c arg m a where random :: (c x arg) => arg -> Random c arg m x? The only thing I can think of is to introduce a new "meta" class ("Affords c arg x"?) to track what constraints imply which arguments...

I think most people looking for a Random effect are going to want something with an off-the-shelf handler to System.Random and/or Crypto.Random (which I should add...)

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