Skip to content

Fix rand(Uniform{Float32})#1355

Closed
oscardssmith wants to merge 1 commit into
JuliaStats:masterfrom
oscardssmith:patch-1
Closed

Fix rand(Uniform{Float32})#1355
oscardssmith wants to merge 1 commit into
JuliaStats:masterfrom
oscardssmith:patch-1

Conversation

@oscardssmith

Copy link
Copy Markdown
Contributor

generate random numbers using the type of the distribution.

generate random numbers using the type of the distribution.
@devmotion

Copy link
Copy Markdown
Member

This seems to be a subset of the changes in #1045. This issue has been raised before but no decision has been made how to deal with the inconsistencies of rand: #1045 (comment) The output type of rand is not always equivalent to the type of the parameters (e.g., continuous distributions with integer valued parameters) and sometimes is difficult to determine in advance (if rand involves some more advanced computations and/or parameters are dual numbers etc.).

@oscardssmith

Copy link
Copy Markdown
Contributor Author

Yeah. I understand that the issue has been raised before, but this is a 1 line change that brings it more in line with Normal and what seemed to be the majority opinion from that issue. I've made this as a separate PR in hopes that it being a 1 line change would make it easier to merge.

@devmotion

Copy link
Copy Markdown
Member

IMO this is not the right approach since it is not a general solution - as I said, sometimes you don't know the output type of rand in advance since the output type can't be deduced in an obvious way from the parameters. Currently, also Normal is based on heuristics (it has to call float(T) since otherwise it would already break for integer-valued parameters). I think it would be better to add an optional type parameter T to rand(rng, dist) that determines the type that is used for underlying rand or randn calls: #1262 (comment)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants