Generic split function#12
Generic split function#12curiousleo wants to merge 1 commit intoidontgetoutmuch:interface-to-performancefrom
Conversation
979cb64 to
c20ebe5
Compare
| , time | ||
| , mtl | ||
| , mwc-random | ||
| , SHA |
There was a problem hiding this comment.
random is a very popular package therefore we should be very careful when adding an extra dependency.
I am not sure that addition of binary here is justified, even though I know that it is a core package.
I am also not sure about SHA dependency here, I quickly benchmarked it against cryptohash-sha256 and the former was 8 times slower.
There was a problem hiding this comment.
There's also problem of GHCJS. I think random is package that must work on GHCJS. cryptohash-sha256 uses C so it wouldn't. It's of course possible to use SHA on GHCJS and cryptohash-sha256 otherwise
There was a problem hiding this comment.
Good point. Thanks. I agree that fallback onto SHA for GHCJS is a good approach
There was a problem hiding this comment.
I'm going to hold off on implementing these suggestions for now. The purpose of this PR was to show the interface changes required (just initialize on RandomGen, as it turns out) and to show one possible implementation.
I created #18 to discuss seeding more generally; a very different design may emerge from it (for example without split being a method on RandomGen) and we may not actually want to use SHA at all.
There was a problem hiding this comment.
That sounds like a good plan. I'll keep this conversation unresolved, just in case.
There was a problem hiding this comment.
Distinguishing splittable from non-splittable was decided to be solved by supplying a really good default implementation for splitting any pure RNG. So far proof-of-concept is implemented in #12
Poor splitting is one of the problems in the current implementation. Where does the proposal for SHA-1 of 8 random bytes come from?
There was a problem hiding this comment.
From here: #7 (comment) - I couldn't find an existing Haskell implementation of SpookyHash and thought that SHA-256 is surely at least as good as a mixing function for a proof-of-concept PR.
|
Ok so the proposal is to have a default split but the implentor of a given RNG would have to explicitly use it (it's not a default in the class definition)? There would need to be some good text in the documentation describing the default and we would encourage (write PRs for?) maintainers of e.g. splitmix and tf-random to give details on their splitting approach? |
Yes, that is the compromise we worked out in #7 (comment) and #7 (comment). Although as I understood @lehins, he wanted to avoid making |
I agree |
|
Regardless of this, I would still vote against having default
|
I also vote against as there is a risk that splitting will not behave as expected. This is the current situation. |
09c76cb to
0aa4051
Compare
|
This PR was based on the idea that |
This makes concrete the idea discussed in #7 (comment).