-
Notifications
You must be signed in to change notification settings - Fork 52
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
Utilities to get a random element from a list and shuffle a list #139
Comments
I support this proposal. From discussion on the FP Discord server, it sounds like a number of other people support it too. There may also be scope to implement other utility functions, not just the two suggested here. I’m not sure what else, though, and those two would be a good place to start. |
These two are the most basic random utilities I can think of and I'd expect any decent random library to have them. I think it makes more sense to make |
Here is a PR with implementation of shuffling: #140 I'd be really happy to see some reviews.
Please don't ask to rename to With respect to random element, I am thinking of a function:
Which can then be used to implement:
@konsumlamm I disagree with you on both points:
It breaks convention of the whole library
First of all |
You're right, it's better to be consistent with the rest of the library.
IMO that's a reason for calling it that, not against.
This is a good point though. |
choose :: (Fodlable f, StatefulGen g m) => f a -> g -> m a
choose f g = do
i <- uniformRM (0,n-1) g
return $ toList f !! i
where
n = length f While |
Yes, I thought about this implementation as well, but that would be sub-optimal for Map and Vector, due to |
Indeed. But it still beats |
I suppose #140 resolves one half of this issue, but the feature hasn't been released yet. Should it be? :) |
What I expect the signatures of these utilities to be:
random-fu
implements the random ekement utility asrandomElement :: [a] -> RVar a
, though it doesn't have to be that specifically. Maybe a morerandom
implementation would berandomElement :: RandomGen g => [a] -> g -> a
?shuffle :: RandomGen g => [a] -> g -> [a]
.I know that there was an issue precisely about this about 3 years ago, but honestly, I think this should be heavily rethought.
These two functionalities are repeatedly provided and reimplemented in many other libraries, with a varying interface, which makes it all the more confusing whether it should be manually implemented by the user, or whether they should add in these libraries (which depend on
random
anyway) just to use these 2 utilities that aren't inrandom
.Forgive me if I'm making assumptions but this seems like this doesn't add many maintenance burdens, and makes dealing with randomness much easier than it is right now. This doesn't seem like it's introducing any sort of feature creep either, it's well within the scope of what
random
does, or at least to me.There are only a few criticisms of this in the other issue, mainly
For the first point, I think that being opinionated in this scenario is the better option than not. So far many libraries have implemented this and we have an overview of how performant/convenient different approaches are. This is aside from the fact that no matter what implementation we choose, we'll be giving a better experience for people than they're having right now. With pretty much an imperceptible number of downsides. If the opinion we make for the user doesn't suit them, then at that point they can make it on their own.
For the second point, I think this is something that most Haskellers have come to expect. Yes, bottoms and laziness will affect implementations. Yes, using linked lists is not recommended as the most performant collection. Even only providing it for linked list will, again, improve people's situations.
For the third point,
random
has already reimplemented behavior from other libraries. It seems to me way more than simply adding two utilities.I'm not really a library maintainer, but as a Haskell programmer... To me, this addition is only an improvement, and it has no downsides.
The text was updated successfully, but these errors were encountered: