Expand documentation of custom random samplers. #31787

tpapp · 2019-04-21T12:23:44Z

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.

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.

tpapp · 2019-04-21T12:29:30Z

One thing that was unclear while I was doing this PR is whether to replace references to eltype by Random.gentype, eg in the docstring of rand! and the Die example. I can easily do this, if someone would clarify the preferred approach for me.

tpapp · 2019-04-26T12:33:49Z

@rfourquet: friendly ping, could you please review at some point? I was hoping this would make it to 1.2.

StefanKarpinski · 2019-04-29T20:30:28Z

We can definitely add docs in an RC.

rfourquet · 2019-04-30T08:27:14Z

@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.

tpapp · 2019-04-30T08:44:34Z

@rfourquet: Thanks for getting back to me. No worries, take your time and get well soon!

rfourquet

Excellent improvement, thank you!

stdlib/Random/docs/src/index.md

rfourquet · 2019-05-03T19:06:34Z

stdlib/Random/docs/src/index.md

-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:


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!)

I tried to clarify this, I hope the improved wording is clear.

stdlib/Random/docs/src/index.md

stdlib/Random/src/Random.jl

rfourquet · 2019-05-03T19:36:09Z

One thing that was unclear while I was doing this PR is whether to replace references to eltype by Random.gentype

You can have a look at the discussion #27756 for background. Basically, people were not super motivated to introduce gentype, as it was seen mostly as a duplicate of eltype. So I kept it as an implementation detail till now. I'm still in favor to make it into the official API, but I think this should get a bit more support. A motivation is for some distributions which are not in Random (which defines only rand for types and collections), e.g. Normal(), or Samplers: these objects are clearly not collections, such that defining eltype on them seems inadequate (hence the introduction of gentype). Your DiscreteDistribution is also such an example.

- 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

tpapp · 2019-05-05T12:43:32Z

@rfourquet: Thanks for the review, I implemented the changes you requested.

Regarding

introduce gentype, as it was seen mostly as a duplicate of eltype. So I kept it as an implementation detail till now. I'm still in favor to make it into the official API, but I think this should get a bit more support.

I think that making gentype part of the API is the right decision, but I leave this for a future PR. Should I open an issue as a reminder?

tpapp · 2019-05-08T07:58:40Z

Friendly bump: please let me know if I should do anything else to get this merged.

StefanKarpinski · 2019-05-08T12:05:47Z

@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

rfourquet · 2019-05-08T15:21:30Z

stdlib/Random/docs/src/index.md

+
+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.


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).

rfourquet · 2019-05-08T15:29:57Z

stdlib/Random/docs/src/index.md

-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`.


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).

stdlib/Random/docs/src/index.md

rfourquet · 2019-05-08T15:38:35Z

@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 gentype: you offered to open an issue, and I think it's a good idea. You can also include content from a comment I just made inline in this PR, or I can add it to your issue.

Co-Authored-By: tpapp <[email protected]>

tpapp · 2019-05-08T16:37:21Z

@rfourquet, thanks for the thorough second review. I addressed all your questions. In particular, I decided to remove all references to Random.gentype so that this can be merged.

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 gentype, I think this is already an improvement.

rfourquet · 2019-05-08T16:48:20Z

stdlib/Random/docs/src/index.md

@@ -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


Here it may be better to replace gentype by eltype.

Ok, adding back.

rfourquet · 2019-05-08T16:51:10Z

Looks better for now with undocumenting gentype. But please check for the (very) few comments you didn't respond to, there is at least one which is important (unqualifed probabilities in make_alias_table(probabilities))

tpapp · 2019-05-08T18:30:16Z

@rfourquet: I don't see a comment about the unqualified probabilities, but I corrected it; also added that eltype for the custom distribution.

This was making Documenter fail.

tpapp · 2019-05-09T10:35:13Z

I fixed the docstring errors (by removing all remaining references to Random.gentype). The remaining CI error seems unrelated.

Unless there is anything else to address, it would be great to finally merge this.

rfourquet · 2019-05-09T15:56:24Z

I don't see a comment about the unqualified probabilities

It's there, along with 4 other comments. Check for "5 hidden conversations" when you reload the page.

Unless there is anything else to address, it would be great to finally merge this.

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.

rfourquet · 2019-05-09T15:57:02Z

Anyway, we can indeed fix that later, thanks for your contribution.

tpapp · 2019-05-10T05:47:27Z

@rfourquet: thanks for pointing these out, I will fix them when I make a PR for #31968.

This was missed in JuliaLang#31787.

fredrikekre requested a review from rfourquet April 21, 2019 23:38

fredrikekre added domain:docs This change adds or pertains to documentation domain:randomness Random number generation and the Random stdlib labels Apr 21, 2019

rfourquet reviewed May 3, 2019

View reviewed changes

minor changes suggested

ad865ce

- 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 reviewed May 8, 2019

View reviewed changes

tpapp and others added 2 commits May 8, 2019 18:34

Consistent ending dots, remove references to gentype.

49961bf

Update stdlib/Random/docs/src/index.md

daf11a5

Co-Authored-By: tpapp <[email protected]>

tpapp mentioned this pull request May 8, 2019

the future of Random.gentype #31968

Open

2 tasks

rfourquet reviewed May 8, 2019

View reviewed changes

define eltype for custom distribution, qualify probabilities

9902f8d

remove all docstring references for gentype

e5a48a1

This was making Documenter fail.

fredrikekre merged commit e813f0d into JuliaLang:master May 9, 2019

tpapp deleted the tp/random-custom-types-explanations branch May 9, 2019 14:06

tpapp added a commit to tpapp/julia that referenced this pull request May 10, 2019

fix typos, clarify what methods should be defined.

be4a77c

This was missed in JuliaLang#31787.

This was referenced May 10, 2019

remove Random.gentype #31988

Closed

document Random samplers: continuation of #31787 #31990

Merged

rfourquet pushed a commit that referenced this pull request May 12, 2019

document Random samplers: continuation of #31787 (#31990)

8932ccd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand documentation of custom random samplers. #31787

Expand documentation of custom random samplers. #31787

tpapp commented Apr 21, 2019 •

edited

Loading

tpapp commented Apr 21, 2019

tpapp commented Apr 26, 2019

StefanKarpinski commented Apr 29, 2019

rfourquet commented Apr 30, 2019

tpapp commented Apr 30, 2019

rfourquet left a comment

rfourquet May 3, 2019

tpapp May 5, 2019

rfourquet commented May 3, 2019

tpapp commented May 5, 2019

tpapp commented May 8, 2019

StefanKarpinski commented May 8, 2019 •

edited

Loading

rfourquet May 8, 2019

rfourquet May 8, 2019

rfourquet commented May 8, 2019

tpapp commented May 8, 2019

rfourquet May 8, 2019

tpapp May 8, 2019

rfourquet commented May 8, 2019

tpapp commented May 8, 2019

tpapp commented May 9, 2019

rfourquet commented May 9, 2019

rfourquet commented May 9, 2019

tpapp commented May 10, 2019


		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.

Expand documentation of custom random samplers. #31787

Expand documentation of custom random samplers. #31787

Conversation

tpapp commented Apr 21, 2019 • edited Loading

tpapp commented Apr 21, 2019

tpapp commented Apr 26, 2019

StefanKarpinski commented Apr 29, 2019

rfourquet commented Apr 30, 2019

tpapp commented Apr 30, 2019

rfourquet left a comment

Choose a reason for hiding this comment

rfourquet May 3, 2019

Choose a reason for hiding this comment

tpapp May 5, 2019

Choose a reason for hiding this comment

rfourquet commented May 3, 2019

tpapp commented May 5, 2019

tpapp commented May 8, 2019

StefanKarpinski commented May 8, 2019 • edited Loading

rfourquet May 8, 2019

Choose a reason for hiding this comment

rfourquet May 8, 2019

Choose a reason for hiding this comment

rfourquet commented May 8, 2019

tpapp commented May 8, 2019

rfourquet May 8, 2019

Choose a reason for hiding this comment

tpapp May 8, 2019

Choose a reason for hiding this comment

rfourquet commented May 8, 2019

tpapp commented May 8, 2019

tpapp commented May 9, 2019

rfourquet commented May 9, 2019

rfourquet commented May 9, 2019

tpapp commented May 10, 2019

tpapp commented Apr 21, 2019 •

edited

Loading

StefanKarpinski commented May 8, 2019 •

edited

Loading