Skip to content
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

How to define random functions on your own types need to be better documented. #25605

Closed
KristofferC opened this issue Jan 17, 2018 · 11 comments
Closed
Assignees
Labels
docs This change adds or pertains to documentation randomness Random number generation and the Random stdlib

Comments

@KristofferC
Copy link
Member

KristofferC commented Jan 17, 2018

and have better error messages.

On 0.6:

julia> struct F
           v::Float64
       end

julia> rand(F)
ERROR: MethodError: no method matching rand(::MersenneTwister, ::Type{F})
Closest candidates are:
  rand(::AbstractRNG, ::Type, ::Tuple{Vararg{Int64,N}} where N) at random.jl:390
  rand(::AbstractRNG, ::Type, ::Integer, ::Integer...) at random.jl:391
  rand(::MersenneTwister, ::Type{I<:Base.Random.FloatInterval}) where I<:Base.Random.FloatInterval at random.jl:143
  ...
Stacktrace:
 [1] rand(::Type{T} where T) at ./random.jl:283

julia> Base.rand(::MersenneTwister, ::Type{F}) = F(rand())

julia> rand(F)
F(0.5219484358707998)

julia> rand(F, 2,2)
2×2 Array{F,2}:
 F(0.0131819)  F(0.0734573)
 F(0.440896)   F(0.735767)

The error message says exactly what is happening, I define the function, everything works.

On 0.7:

julia> struct F
           v::Float64
       end

julia> rand(F)
ERROR: ArgumentError: Sampler for this object is not defined
Stacktrace:
 [1] Type at ./random/random.jl:106 [inlined]
 [2] rand at ./random/random.jl:193 [inlined]
 [3] rand at ./random/random.jl:194 [inlined]
 [4] rand(::Type{F}) at ./random/random.jl:198
 [5] top-level scope

Ok... what do I do... Looking at docs for rand, says nothing about Sampler. Looking at NEWS.md, nothing about Sampler. Looking at docs, nothing about Sampler. Looking at >help? Sampler, No documentation found.. Let's try the same as in 0.6:

julia> Base.rand(::MersenneTwister, ::Type{F}) = F(rand())

julia> rand(F)
F(0.15546036169717214)

Woho, it works... or?

julia> rand(F, 2, 2)
ERROR: ArgumentError: Sampler for this object is not defined
Stacktrace:
 [1] Type at ./random/random.jl:106 [inlined]
 [2] rand at ./random/random.jl:193 [inlined]
 [3] rand! at ./random/random.jl:210 [inlined]
 [4] rand! at ./random/random.jl:206 [inlined]

Gives up.

@JeffBezanson JeffBezanson added docs This change adds or pertains to documentation randomness Random number generation and the Random stdlib labels Jan 17, 2018
@JeffBezanson
Copy link
Member

This is a fine example of a general theme I've seen a few times. Sometimes people feel a MethodError is too "low level" and user-unfriendly, and that a more application-specific error message should be substituted for it. But I generally think that it's good to know how the system works, in which case a MethodError is actually quite informative and more useful than an ad-hoc message: the formatting is consistent so you get better at reading MethodErrors over time, and more effort has been put in to MethodErrors to make them include more information.

@KristofferC
Copy link
Member Author

KristofferC commented Jan 18, 2018

MbedTLS fails with this error message now: https://travis-ci.org/JuliaWeb/GitHub.jl/jobs/330410030#L1289.

Still no idea how to fix. Was this supposed to be just an optimization or an API change? If purely optimization, should be fixed to not break code, if API change, should have deprecation and NEWS:

@stevengj
Copy link
Member

I agree with @KristofferC that the issue here is not MethodError at all. The issue is:

  • Sampler etc should be documented if is needed to extend rand
  • The problem here is that the ArgumentError (not a MethodError!) is obscure and needs to be rewritten/replaced.

@rfourquet
Copy link
Member

I will address the issue.

@rfourquet rfourquet self-assigned this Jan 19, 2018
@rfourquet
Copy link
Member

The error message says exactly what is happening, I define the function, everything works.

The previous rand system could be specialized for custom types, but it was not designed for extension nor documented as such. Sure, you could get away with defining ad-hoc functions according to the MethodError messages, but you would end-up re-inventing the wheel in some cases. E.g. (julia v0.6):

julia> Base.rand(::MersenneTwister, f::F) = rand()*f.v

julia> julia> rand(MersenneTwister(0), F(1.1))
0.7166581394375923

julia> julia> rand(F(2.0), 2, 2)
ERROR: MethodError: no method matching rand(::F, ::Int64, ::Int64)
Closest candidates are:
  rand(::Integer...) at random.jl:280
  rand(::Type, ::Integer, ::Integer...) at random.jl:282
  rand(::AbstractArray, ::Integer...) at random.jl:297
  ...

and you now have to define a loop yourself for array generation.

Gives up.

Yes, I fully agree the new system needs documentation and a better error message. In the PR introducing Sampler, I had of course the intent to document this eventually (before the 0.7 release), but it was still experimental (with of course all base tests passing), and I felt this was useful to have this system used for some time in Base during the 0.7 timeframe, to catch potential design mistake and/or find improvement opportunities.

@rfourquet
Copy link
Member

Sometimes people feel a MethodError is too "low level" and user-unfriendly, and that a more application-specific error message should be substituted for it

This is not what happened here, cf. #23964 (comment)

@KristofferC
Copy link
Member Author

Bump, what is the status on this? I would like to reiterate how awesome it would be if this could be made non-breaking.

@fredrikekre
Copy link
Member

Bump; documentation for this is very much needed now that we start upgrading to v0. 7.

@rfourquet
Copy link
Member

Bump; documentation for this is very much needed now that we start upgrading to v0. 7.

Message received!

@rfourquet
Copy link
Member

I would like to reiterate how awesome it would be if this could be made non-breaking.

I believe this is not possible while keeping the benefits of the new framework. I would like to reiterate how limited the previous API was; I know you are not a big fan of this change, but hope you will come to appreciate the new possibilities.

@zsunberg
Copy link
Contributor

It's still kind of difficult to find and understand the documentation for this. If I google "ERROR: ArgumentError: Sampler for this object is not defined", it brings me here.

When I finally found the right part of the docs (https://docs.julialang.org/en/v1/stdlib/Random/#Generating-values-from-a-collection-1), I was very surprised to find that, instead of learning how to implement a Sampler, I needed to implement rand Random.rand(rng::AbstractRNG, st::Random.SamplerTrivial{MyDist}). I think the "Sampler for this object is not defined" error message could be greatly improved to point users more quickly toward a way to fix the problem :)

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

No branches or pull requests

6 participants