Skip to content

Convert std.random to a package #3406

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

Closed
wants to merge 10 commits into from

Conversation

WebDrake
Copy link
Contributor

As mooted at DConf, this PR converts std.random into a package layout clearly distinguishing between the different types of functionality available:

  • two different varieties of random number generator:
    • random engines, sources of uniformly-distributed pseudo-random bits;
    • random devices, sources of uniformly-distributed non-deterministic random bits;
  • random distributions, which transform uniformly-distributed random bits into variates with other statistics;
  • random algorithms, i.e. algorithms whose outcome depends on some random component.

A std.random.traits module has also been included to house RNG-related template checks.

@WebDrake
Copy link
Contributor Author

Apologies for the lack of rebase, fixing that now.

@WebDrake WebDrake force-pushed the std-random-package branch from 132077c to 7ab6874 Compare June 12, 2015 21:03
@WebDrake
Copy link
Contributor Author

Rebased. Just my luck to be pushing this PR as the design of posix.mak was being updated!

I may have accidentally lost Lionello's recent minor Ddoc patch in the process, but no doubt that can be sorted out in the wash.

WebDrake added 10 commits June 23, 2015 13:27
This patch does the most basic work of converting std.random to being
a package, with corresponding updates to the makefiles.
This patch introduces the intended module structure for the new package,
according to the following classifications of functionality:

  * random _engines_, i.e. sources of uniformly distributed
    pseudo-random bits;

  * random _devices_, i.e. sources of uniformly distributed
    non-deterministic random bits;

  * random _distributions_, which transform random bits into variates
    with other statistical properties;

  * random _algorithms_, i.e. algorithms whose outcome has a random
    component;

  * traits for use with random number generation.

As a first step, the new modules have been created without copying any
code, merely providing a module header and documentation.  The makefiles
have been updated accordingly.
As far as I can tell, the code which this comment referred to was
introduced with the very first implementation of std.random in commit
cf1172c, and the comment has persisted
despite the code it referred to long being removed.
@WebDrake
Copy link
Contributor Author

OK, with no feedback so far, I'm going to try to fix up the Windows makefiles. In the absence of my own Windows setup, I'm going to rebase and push one patch at a time, making sure that things pass. Apologies for the abuse of the testing framework ... !

@WebDrake WebDrake force-pushed the std-random-package branch from 7ab6874 to 0231519 Compare June 23, 2015 11:38
@WebDrake
Copy link
Contributor Author

Remaining patches will be pushed tomorrow.

@DmitryOlshansky
Copy link
Member

In the absence of my own Windows setup, I'm going to rebase and push one patch at a time, making sure that things pass. Apologies for the abuse of the testing framework ... !

It's an absolutely valid use case. ;)

@WebDrake
Copy link
Contributor Author

It's an absolutely valid use case. ;)

Good to know :-)

The last of the code-changing patches has now been pushed -- the remainder are documentation and comment tweaks.

Since this PR is technically creating new modules, do I need to request it be added to the Review Queue? http://wiki.dlang.org/Review_Queue

@DmitryOlshansky
Copy link
Member

Since this PR is technically creating new modules, do I need to request it be added to the Review Queue? http://wiki.dlang.org/Review_Queue

Don't think so. Packaging modules is non-controversial thing we are doing for some time now. A discussion on forums might be in order though, just to get some attention.

@WebDrake
Copy link
Contributor Author

Don't think so. Packaging modules is non-controversial thing we are doing for some time now. A discussion on forums might be in order though, just to get some attention.

Great, I'll do that once I've finished pushing all the patches. Thanks very much!

@WebDrake
Copy link
Contributor Author

Last patch has been pushed. I think this is now 100% ready for review.

@WebDrake
Copy link
Contributor Author

Most obvious immediate issue from my point of view: the documentation for modules seems a bit screwy, as the individual methods of various structs are now being given their own documentation pages. Not sure if this is intentional or accidental, it looks overkill to me.

@jkm
Copy link
Contributor

jkm commented Jun 24, 2015

You are changing some Phobos modules that used to import std.random. Are these changes necessary? I'm asking because I assumed that doing public import inside the package.d is enough. But it seems it is not. Hence, this may cause external code to break. Not that I'm against this kind of code breakage but we should make a note for that.
You are talking about the documentation looking odd. Where can I find that documentation?

@DmitryOlshansky
Copy link
Member

But it seems it is not. Hence, this may cause external code to break. Not that I'm against this kind of code

Importing sub-package modules is better b/c of pulling in less dependencies. It's not required but strongly recomened for any library module to reduce its dependencies to bare minimum. Phobos is a library.

@WebDrake
Copy link
Contributor Author

You are changing some Phobos modules that used to import std.random. Are these changes necessary?

No, they are not necessary, but it seemed in line with current phobos practice to prefer importing in this more fine-grained way, for the reasons @DmitryOlshansky has already mentioned.

I'm asking because I assumed that doing public import inside the package.d is enough.

It is sufficient. You can import std.random as previously and things will Just Work (if you don't believe me, note that all tests pass for the patches preceding the patch that tweaks other phobos modules).

Hence, this may cause external code to break.

It should not cause any external code breakage.

You are talking about the documentation looking odd. Where can I find that documentation?

I'm talking about the documentation that the auto-tester built:
http://dtest.thecybershadow.net/results/ad2993ba2c7a8e46a4d31b07369452e7ec6973f9/e4c96258d5e3ae592f8d39f1a97f4f02b793d140/

In particular, I find it odd that it now generates pages like this:
http://dtest.thecybershadow.net/artifact/website-0cd1d419852b8fa39d4be5e1a1aaeee4b432c069-a151eba4d14a5badff48521542443cac/web/library-prerelease/std/random/algorithm/random_sample.pop_front.html

@jkm
Copy link
Contributor

jkm commented Jun 24, 2015

I see. I didn't look at the individual commits. Regarding the generated documentation: I think it looks odd because some functions are documented in the old style. You can fix the documentation if you want.

@jkm
Copy link
Contributor

jkm commented Jun 24, 2015

One more question. Am I right that the only thing that makes unpredictableSeed non-deterministic is that is calls Thread.getThis()?

@WebDrake
Copy link
Contributor Author

One more question. Am I right that the only thing that makes unpredictableSeed non-deterministic is that is calls Thread.getThis()?

I think the use of TickDuration adds a further non-deterministic element. But it's not really a matter of consideration for this PR, I think, as the functionality is not being changed.

@WebDrake
Copy link
Contributor Author

BTW, Jonathan M. Davis raised this interesting objection to the PR as it stands:
http://forum.dlang.org/post/yznzoncjukyzhfyrypqs@forum.dlang.org

@andralex
Copy link
Member

I think all other std/*.d should be left alone by this PR. If anything they'd stand as some proof that the changes don't break things.

@jkm
Copy link
Contributor

jkm commented Jun 25, 2015

One more question. Am I right that the only thing that makes unpredictableSeed non-deterministic is that is calls Thread.getThis()?

I think the use of TickDuration adds a further non-deterministic element. But it's not really a matter of consideration for this PR, I think, as the functionality is not being changed.

I'm unsure what's your definition of non-deterministic? And it is somewhat important because unpredictableSeed is placed into devices.

@jkm
Copy link
Contributor

jkm commented Jun 25, 2015

I wasn't at dconf. But Jonathan is right that it seems to be the better option to first iron out how random should look like and then later decide on migration/deprecation. This allows you to do a less constrained design (results in a better design in the end). In my opinion go for std.experimental.random.

@WebDrake
Copy link
Contributor Author

I think all other std/*.d should be left alone by this PR. If anything they'd stand as some proof that the changes don't break things.

Fair enough.

Re the general PR: I'm probably going to be slow following up on this anyway (there have been some busy weeks and more are anticipated). So let's let this lie for now, and I'll try and follow up with some more concrete "next-gen" code (even if it's just a proof-of-concept). Then we can decide how to proceed. If people want the PR closed until there is follow-up, that's fine.

@WebDrake
Copy link
Contributor Author

Regarding unpredictableSeed: I feel its placement in std.random.device is more than anything else a statement of intent. That method should be non-deterministic, whatever the current implementation may be.

@wilzbach
Copy link
Contributor

Ping @WebDrake - I think your idea to reorganize random is great! Do you have some time to push it through?

How about submitting your changes to the other modules in a follow-up PR?

@wilzbach
Copy link
Contributor

I am closing this now with regrets due to its inactivity and lack of interest.
In the meantime mir-random was built and it seems a good solution to test APIs and then move it forward to std.experimental if someone wants to push it (the current attitude of the Mir team is that maintaining DUB packages is a lot less effort and provides more flexibility)

@WebDrake Feel free to resubmit this if you want to push this - otherwise your contributions to mir-random are equally welcome :)

@wilzbach wilzbach closed this Dec 26, 2016
@WebDrake
Copy link
Contributor Author

@wilzbach I think closing this is the right decision; TBH I don't think it should have stayed open for so long. The reason for the lack of inactivity isn't unwillingness to make it happen per se, but the feedback from @jmdavis that it would be better to solve the foundational problems of std.random before breaking it out into a package. I agreed with that feedback, otherwise this PR would have been completed long ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants