-
-
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
deprecate rand(::Tuple) #25429
deprecate rand(::Tuple) #25429
Conversation
1d8e045
to
8561baf
Compare
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.
lgtm! Thanks @rfourquet! :)
base/deprecated.jl
Outdated
@@ -1138,6 +1138,10 @@ import .Random: srand | |||
@deprecate srand(filename::AbstractString, n::Integer=4) srand(read!(filename, Vector{UInt32}(uninitialized, Int(n)))) | |||
@deprecate MersenneTwister(filename::AbstractString) srand(MersenneTwister(0), read!(filename, Vector{UInt32}(uninitialized, Int(4)))) | |||
|
|||
# PR #25429 | |||
@deprecate rand(r::AbstractRNG, dims::Dims) rand(r, Float64, dims) |
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.
Does this one really need to be deprecated? I thought a dims tuple as the second argument was ok (and there are still some other methods with that).
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.
in rand([rng], [S], [dims])
, the conflict is when using t::Tuple
either for S
or for dims
, whether rng
is specified or not. In rand(rng, (1, 2, 3))
, the conflict is the same as in rand((1, 2, 3))
. A tuple as the second argument is ok when rng
is not specifed, as in rand(Float64, (1, 2, 3))
.
and there are still some other methods with that
I must have missed that, but no test catched this apparently...
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 see. In that case this seems fine as-is.
95c64fd
to
c4aba4d
Compare
The Appveyor failure seems to be a timeout (the job lasted 2 hours), and the travis failure is a problem with |
Pre-requisite for #25278.