Skip to content

Random: document the Sampler machinery #27983

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

Merged
merged 4 commits into from
Jul 27, 2018
Merged

Random: document the Sampler machinery #27983

merged 4 commits into from
Jul 27, 2018

Conversation

rfourquet
Copy link
Member

This is a first draft which is not ready for review, but I thought I would still share it for potential eager reviewers.

I will have very little time in the next few weeks, so my plan is to merge this by the middle of next week to support upgradathon, and work on possible improvements later.

Fixes #25605.

@rfourquet rfourquet added docs This change adds or pertains to documentation randomness Random number generation and the Random stdlib labels Jul 7, 2018
@rfourquet
Copy link
Member Author

This is in better shape now (though not yet ideal) and ready for review. Note that in 1 example (rand(Die, 3)), current master will give a differently typed array (Any instead of Die), this will be fixed in another PR that I'm about to merge soon.

@rfourquet rfourquet changed the title [WIP] Random: document the Sampler machinery Random: document the Sampler machinery Jul 9, 2018
@rfourquet
Copy link
Member Author

Is it expected that circleci is "pending" for many hours (almost one day). Also I don't really understand Appveyor failures, could someone have a look and confirm that these are not related to this PR?

@KristofferC
Copy link
Member

Download is shaky lately, and also #27982. Doesn't seem related.

@rfourquet
Copy link
Member Author

Ok thank you for having a look! Given that it's only a documentation PR, re-running the tests only for appveyor seems unnecessary, when (if I understood correctly) doctests are mostly run on travis. This is good to go for me, feel free to merge.

struct Die
nsides::Int # number of sides
end

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should add

using Random

to prevent deprecations warning

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a bunch of stdlib modules assume an implicit corresponding using directive...

Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

I think making the examples doctests would be very nice to prevent them from going stale.

Let's take the following example: we implement a `Die` type, with a variable number `n` of sides, numbered from `1` to `n`.
We want `rand(Die)` to produce a die with a random number of up to 20 sides (and at least 4):

```julia
Copy link
Member

Choose a reason for hiding this comment

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

Can this plus code below be doctests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will see what I can do. What I find slightly annoying for rand examples is that you have to explicitly use srand, but not a big deal. Also, I was not clear how to re-use a definition in block into another block, but IIRC, it's quite easy.

Copy link
Member

Choose a reason for hiding this comment

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

You can call srand in a setup block e.g.

```jldoctest; setup = :(using Random; srand(1234))
julia> rand()
1.2
```

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

`rand(rng::AbstractRNG, sp::Random.SamplerTrivial{T})`. Here, `sp` simply wraps an object of type `T`, which can be accessed via `sp[]`.
Continuing the `Die` example, we want now to define `rand(d::Die)` to produce an `Int` corresponding to one of `d`'s sides:

```julia-repl
Copy link
Member

Choose a reason for hiding this comment

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

Can be doctest?


#### Generating values from a collection

Given a collection type `T`, it's currently assumed that if `rand(::T)` is defined, an object of type `eltype(T)` will be produced.
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth using another letter than T here to not mix it up with the T earlier used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears sufficiently far from the other T, but if you give me a new name for here, I will use it.

Copy link
Member

Choose a reason for hiding this comment

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

S

```@meta
DocTestSetup = nothing
DocTestSetup = :(using Random)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found it at the end of the file, so left it there.

Copy link
Member

Choose a reason for hiding this comment

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

But why change nothing to using Random?

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this practice in another stdlib and assumed this would be necessary for doctests... I guess I was planing on making doctests at that time ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Well, we have this setup with using Random at the top of this file, and here at the bottom we want to reset it, so we put it to nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

oups!! embarassing, will fix

@rfourquet
Copy link
Member Author

Added jldoctest blocks. I will merge in a couple of days in no more change suggestions.

@rfourquet rfourquet merged commit 748ee1b into master Jul 27, 2018
@rfourquet rfourquet deleted the rf/rand/doc branch July 27, 2018 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants