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

Fix rand(Uniform{Float32}) #1355

Closed
wants to merge 1 commit into from

Conversation

oscardssmith
Copy link

generate random numbers using the type of the distribution.

generate random numbers using the type of the distribution.
@devmotion
Copy link
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
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
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