RFC: Restricts parameter type for randn & randexp to Float types for #17608. #17627

NHDaly · 2016-07-26T07:54:07Z

Fixes base/random.jl and adds tests to tests/random.jl to restrict parametrized type parameter to one of Float16,Float32,Float64.

These functions already worked only for those types, but erroneously allowed parameters outside those three, with unintended behavior (crashing and infinite-looping). Now, this throws a MethodError if provided a non-float type-parameter.

tkelman · 2016-07-26T08:11:03Z

Very nice, thank you! cc @rfourquet
Same issue for BigFloat, right? May as well throw that into the tests?

NHDaly · 2016-07-26T08:23:55Z

Done. Although, would we want to consider just adding BigFloat to the list of supported types for randn and randexp? I.e., adding it to the Floats union?

I can leave that for another PR after this bug-fix though. Should I leave a TODO somewhere for it?

tkelman · 2016-07-26T08:26:46Z

I imagine there's code somewhere in MPFR for those, but I don't know whether we have the ccall wrappers here yet. I think a TODO as an issue, a code/test comment, or an early WIP PR would be fine.

NHDaly · 2016-07-26T08:32:36Z

I've added code TODOs. Can you verify that I've formatted them in a reasonable way? (I'm new to JuliaLang contributing and don't know your conventions). :)

(Should I have written # TODO(17627): instead of # TODO(#17627):?)

tkelman · 2016-07-26T08:34:29Z

3c91c92 looks fine to me, we're not super uniform in formatting of todo's and fixme's

NHDaly · 2016-07-26T08:39:30Z

S.g. Thanks for the review! :)

rfourquet · 2016-07-26T09:34:45Z

test/random.jl

+
+        @test_throws MethodError r(rng..., Number, (2,3))
+        @test_throws MethodError r(rng..., Any, 1)
+    end


I would add a second loop for concision:

for T in [Int, Int32, String, AbstractFloat, BigFloat, Number, Any] @test_throws MethodError r(rng..., T) @test_throws MethodError r(rng..., T, 1) @test_throws MethodError r(rng..., T, (2, 3)) end

(and, why not, a loop over the dimensions: for d in ([], [1], [(2, 3)])).

rfourquet · 2016-07-26T11:42:17Z

Another possible fix would be to write a signature similar to what I used there to fix a similar problem, but I guess it makes sense to restrict those randn functions to AbstractFloat anyway (cf. my inline comment for using AbstractFloat instead of $Floats).
Also you may squash the commits when done.
Otherwise LGTM.

stevengj · 2016-07-26T14:16:09Z

base/random.jl

+            $randfun{T<:$Floats}(rng::AbstractRNG, ::Type{T}, dims::Dims)       = $randfun!(rng, Array{T}(dims))
+            $randfun{T<:$Floats}(rng::AbstractRNG, ::Type{T}, dims::Integer...) = $randfun!(rng, Array{T}(dims...))
+            $randfun{T<:$Floats}(                  ::Type{T}, dims::Dims)       = $randfun(GLOBAL_RNG, T, dims)
+            $randfun{T<:$Floats}(                  ::Type{T}, dims::Integer...) = $randfun(GLOBAL_RNG, T, dims...)


I don't like these restrictions either, for the same reason as above. It makes it significantly harder to define e.g. randn for new types.

stevengj · 2016-07-26T14:27:23Z

It seems like a better solution is much simpler. Just change:

$randfun{T}(rng::AbstractRNG, ::Type{T}, dims::Integer...) = $randfun!(rng, Array{T}(dims...))

to

$randfun{T}(rng::AbstractRNG, ::Type{T}, dim0::Integer, dims::Integer...) = $randfun!(rng, Array{T}(dim0, dims...))

That will remove the circular fallback definition for $randfun{T}(rng::AbstractRNG, ::Type{T}), without making it harder to define randn for new types.

rfourquet · 2016-07-26T14:31:55Z

Ah of course, I had already forgotten about randn for Complex. So I concur with your proposed solution (identical to what I linked to above).

NHDaly · 2016-07-28T04:06:45Z

Hi, yeah that makes a ton of sense. This is a great way to break the recursion, thanks. I wasn't sure what was the best way, so I just tried the first one that came to mind. :) Thanks!

I'll try this tomorrow and send the PR back to you. Sorry for the delay; i don't have my machine with me. Thanks!

stevengj · 2016-07-28T14:39:58Z

@NHDaly, note that you should just push a new commit to this PR, rather than opening a brand-new PR.

The functions already only supported Float types (Float16,Float32,Float64), but could be called with non-float types. Calling with Non-Float types, e.g. `Int`, could cause an infinite loop. Also adds tests verifying that randn throws a MethodError for types besides Float types.

NHDaly · 2016-07-28T15:03:59Z

@stevengj Yep, thanks! I've synced, rebased, and squashed.

This solution definitely looks better, and the tests still pass on my client. Lmk what you think! Thanks

stevengj · 2016-07-28T15:06:40Z

base/random.jl

-            $randfun(                                dims::Dims)       = $randfun(GLOBAL_RNG, Float64, dims)
-            $randfun(                                dims::Integer...) = $randfun(GLOBAL_RNG, Float64, dims...)
+            $randfun{T}(rng::AbstractRNG, ::Type{T}, dims::Dims                     ) = $randfun!(rng, Array{T}(dims))
+            $randfun{T}(rng::AbstractRNG, ::Type{T}, dim0::Integer, dims::Integer...) = $randfun!(rng, Array{T}(dim0, dims...))


Probably dim1 would be more Julian (1-based).

Done. Thanks!

NHDaly · 2016-07-29T00:39:33Z

I've left some inline comments. PTAL. thanks! :)

stevengj · 2016-07-29T01:53:28Z

As noted above, I would strongly prefer that, to add randn for a new type, you should only have to define randn(rng::AbstractRNG, ::Type{T}). This means making the one-argument randn work for any type, not just Floats.

NHDaly · 2016-07-29T03:21:43Z

:) Okay I'm sold! I agree, you should only have to define randn(rng::AbstractRNG, ::Type{T}) for your type. So I've made the one-arg version work for any type. Thanks!

Let me know if this looks okay and I'll squash the commits.

stevengj · 2016-07-29T14:56:31Z

LGTM.

stevengj · 2016-07-29T14:56:55Z

(No need to squash the commits, the Github merge button can do that for us.)

NHDaly · 2016-07-29T16:05:35Z

(No need to squash the commits, the Github merge button can do that for us.)

:O Woahh fancy! :) cool thanks!

…uliaLang#17608. (JuliaLang#17627) * Fixes infinite recursion in randn for non-floats; fixes JuliaLang#17608. The functions already only supported Float types (Float16,Float32,Float64), but could be called with non-float types. Calling with Non-Float types, e.g. `Int`, could cause an infinite loop. Also adds tests verifying that randn throws a MethodError for types besides Float types. * s/dim0/dim1/ * removed other dimensions, fixed randn(T), added comment.

NHDaly mentioned this pull request Jul 26, 2016

implement randn!(::Array{Float32}) etc. (fix #9836) #14811

Merged

tkelman added the randomness Random number generation and the Random stdlib label Jul 26, 2016

NHDaly mentioned this pull request Jul 26, 2016

Consider adding BigFloat support for randn/randexp #17629

Closed

rfourquet reviewed Jul 26, 2016
View reviewed changes

stevengj reviewed Jul 26, 2016
View reviewed changes

stevengj reviewed Jul 28, 2016
View reviewed changes

s/dim0/dim1/

3e1b606

removed other dimensions, fixed randn(T), added comment.

7cd7dce

stevengj merged commit e70de21 into JuliaLang:master Jul 29, 2016

stevengj mentioned this pull request Jul 29, 2016

Infinite recursion in randn(Int) #17608

Closed

NHDaly deleted the randn_domain_fix branch August 2, 2016 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Restricts parameter type for randn & randexp to Float types for #17608. #17627

RFC: Restricts parameter type for randn & randexp to Float types for #17608. #17627

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

rfourquet Jul 26, 2016

rfourquet commented Jul 26, 2016

stevengj Jul 26, 2016

stevengj commented Jul 26, 2016 •

edited

Loading

rfourquet commented Jul 26, 2016

NHDaly commented Jul 28, 2016

stevengj commented Jul 28, 2016

NHDaly commented Jul 28, 2016

stevengj Jul 28, 2016

NHDaly Jul 29, 2016

NHDaly commented Jul 29, 2016

stevengj commented Jul 29, 2016 •

edited

Loading

NHDaly commented Jul 29, 2016

stevengj commented Jul 29, 2016

stevengj commented Jul 29, 2016

NHDaly commented Jul 29, 2016

RFC: Restricts parameter type for randn & randexp to Float types for #17608. #17627

RFC: Restricts parameter type for randn & randexp to Float types for #17608. #17627

Conversation

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

tkelman commented Jul 26, 2016

NHDaly commented Jul 26, 2016

rfourquet Jul 26, 2016

Choose a reason for hiding this comment

rfourquet commented Jul 26, 2016

stevengj Jul 26, 2016

Choose a reason for hiding this comment

stevengj commented Jul 26, 2016 • edited Loading

rfourquet commented Jul 26, 2016

NHDaly commented Jul 28, 2016

stevengj commented Jul 28, 2016

NHDaly commented Jul 28, 2016

stevengj Jul 28, 2016

Choose a reason for hiding this comment

NHDaly Jul 29, 2016

Choose a reason for hiding this comment

NHDaly commented Jul 29, 2016

stevengj commented Jul 29, 2016 • edited Loading

NHDaly commented Jul 29, 2016

stevengj commented Jul 29, 2016

stevengj commented Jul 29, 2016

NHDaly commented Jul 29, 2016

stevengj commented Jul 26, 2016 •

edited

Loading

stevengj commented Jul 29, 2016 •

edited

Loading