-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Expand documentation of custom random samplers. #31787
Expand documentation of custom random samplers. #31787
Conversation
Following discussion on Discourse, this PR expands the documentation for defining custom random samplers. Motivation is provided for the design, followed by examples that gradually introduce features and explain why/when they should be used. No text was removed, but some parts were moved to other subsections. Docstrings added for relevant methods and structures, and included in the docs. The use of pre-existing types is emphasized.
One thing that was unclear while I was doing this PR is whether to replace references to |
@rfourquet: friendly ping, could you please review at some point? I was hoping this would make it to 1.2. |
We can definitely add docs in an RC. |
@tpapp I'm sorry this takes so long, the improvement here is amazing. I combined being on hollidays and sick, and couldn't find a moment to properly review. I should be able to manage by the end of the week. |
@rfourquet: Thanks for getting back to me. No worries, take your time and get well soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent improvement, thank you!
stdlib/Random/docs/src/index.md
Outdated
In order to implement this decoupling for a custom type, a helper type can be used. | ||
Going back to our `Die` example: `rand(::Die)` uses random generation from a range, so | ||
there is an opportunity for this optimization: | ||
In order to implement this decoupling for a custom type, a custom helper type can be used. Going back to our `Die` example: `rand(::Die)` uses random generation from a range, so there is an opportunity for this optimization: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should change "In order to implement this decoupling for a custom type", as you already showed how to do the decoupling for a custom type... maybe add "in cases where SamplerSimple
is not suitable? (but the problem then is that we explain SamplerSimple
was suitable in our Die
example!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to clarify this, I hope the improved wording is clear.
You can have a look at the discussion #27756 for background. Basically, people were not super motivated to introduce |
- clarify role of `gentype` - use `sp` as an argument name for samplers - corrections for docstrings - clarify pedagogical motivation for custom samplers not using existing types - various minor text corrections: spaces, emphases
@rfourquet: Thanks for the review, I implemented the changes you requested. Regarding
I think that making |
Friendly bump: please let me know if I should do anything else to get this merged. |
@rfourquet, please merge whenever it looks good to you. Doesn't have to be perfect, we can always improve after the fact. |
stdlib/Random/docs/src/index.md
Outdated
|
||
3. `SamplerSimple(self, data)` also contains the additional `data` field, which can be used to store arbitrary pre-computed values. | ||
|
||
In addition, [`Random.gentype`](@ref), which falls back to [`eltype`](@ref) is used for computing element types for containers. A method should be defined in case `eltype` is not defined, or a different element type is desired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused here: you said
making gentype part of the API is the right decision, but I leave this for a future PR
with which I agree, but you document it now, making it public API...
If we keep a reference to gentype
here in this document, we would need the approval of a core dev... and a rework of this sentence which is still not fully satisfying: the raison-d'être of gentype
is to handle non-collections: collections have already eltype
defined by definition, so saying gentype
is used for computing element types for containers reinforces the idea that gentype
is redundant. Moreover, the second sentence suggests that its fine to have eltype != gentype
, which I don't think we should encourage... actually, I may even tend to believe this should not be allowed, i.e. either eltype
or gentype
should be defined, but not both... which is a point in favor of removing gentype
altogether, while extending the scope of eltype
(so that it's applicable beyond collections).
stdlib/Random/docs/src/index.md
Outdated
Given a type `T`, it's currently assumed that if `rand(T)` is defined, an object of type `T` will be produced. | ||
In order to define random generation of values of type `T`, the following method can be defined: | ||
`rand(rng::AbstractRNG, ::Random.SamplerType{T})` (this should return what `rand(rng, T)` is expected to return). | ||
Given a type `T`, it's currently assumed that if `rand(T)` is defined, an object of type `T` will be produced. `SamplerType` is the *default sampler for types*. In order to define random generation of values of type `T`, the `rand(rng::AbstractRNG, ::Random.SamplerType{T})` method should be defined, and should return values what `rand(rng, T)` is expected to return. `Random.gentype` should *not* need to be defined explicitly for this case, as the type is `T`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep a reference to gentype
here, I suggest something like:
"Note that in this case, Random.gentype
must not be defined (generated values are of type T
).
@tpapp thanks for the updates, I added a few more comments. But you seem to have missed few of my comments from the first batch? Concerning |
Co-Authored-By: tpapp <tkpapp@gmail.com>
@rfourquet, thanks for the thorough second review. I addressed all your questions. In particular, I decided to remove all references to In agreement with @StefanKarpinski, I would prefer to get this PR merged, with the understanding that it can always be improved later. Regardless of what happens to |
stdlib/Random/docs/src/index.md
Outdated
@@ -198,8 +195,6 @@ end | |||
``` | |||
and that we *always* want to build an a alias table, regardless of the number of values needed (we learn how to customize this below). The methods | |||
```julia | |||
Random.gentype(::Type{<:DiscreteDistribution}) = Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it may be better to replace gentype
by eltype
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, adding back.
Looks better for now with undocumenting |
@rfourquet: I don't see a comment about the unqualified |
This was making Documenter fail.
I fixed the docstring errors (by removing all remaining references to Unless there is anything else to address, it would be great to finally merge this. |
It's there, along with 4 other comments. Check for "5 hidden conversations" when you reload the page.
I didn't miss the fact that you wished this to be merged without unnecessary delay, I reviewed this PR at the pace I could. There is something left to address (cf. the "4 comments" mentioned above), but unfortunately it's already merged. |
Anyway, we can indeed fix that later, thanks for your contribution. |
@rfourquet: thanks for pointing these out, I will fix them when I make a PR for #31968. |
This was missed in JuliaLang#31787.
Following discussion on Discourse with @rfourquet, this PR expands the documentation for defining custom random samplers. Motivation is provided for the design, followed by examples that gradually introduce features and explain why/when they should be used.
No text was removed, but some parts were moved to other subsections.
Docstrings added for relevant methods and structures, and included in the docs.
The use of pre-existing types is emphasized.