-
-
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
Add complex randn #21973
Add complex randn #21973
Conversation
base/random.jl
Outdated
@@ -1194,6 +1196,9 @@ The `Base` module currently provides an implementation for the types | |||
end | |||
end | |||
|
|||
Base.@irrational SQRT_HALF 0.7071067811865475244008 sqrt(big(0.5)) | |||
randn{T}(rng::AbstractRNG, ::Type{Complex{T}}) = Complex{T}(SQRT_HALF * randn(rng, T), SQRT_HALF*randn(rng, T)) |
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.
For new code, maybe use the where
syntax. Also, don't you want to restrict T<:AbstractFloat
?
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.
Ah right I forgot about the new where
syntax. Good call about restriction at the call site. I wasn't sure if should just let it blow up at the call the randn(rng, T)
, but it does makes the error message a little more cryptic doesn't it.
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 think it would make an InexactError
when calling the complex constructor on real (because of the multiplication by SQRT_HALF
) values, e.g. with T==Int
. Also, this distribution will make sense only when randn(T)
produces a number in [0,1)
, i.e. T<:AbstractFloat
.
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'm not sure I understand what you mean by this. Can you explain another way?
LGTM. |
base/random.jl
Outdated
|
||
# complex randn | ||
Base.@irrational SQRT_HALF 0.7071067811865475244008 sqrt(big(0.5)) | ||
randn(rng::AbstractRNG, ::Type{Complex{T}}) where {T <: Floats} = Complex{T}(SQRT_HALF * randn(rng, T), SQRT_HALF*randn(rng, T)) |
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'm not sure if we want T<:Floats
or T<:AbstractFloat
. E.g. if someone defines randn(::CustomRNG, ::BigFloat)
, she won't benefit from your complex randn
definition...
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.
Should be AbstractFloat
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.
Done, also added a statement to the NEWS
cc @stevengj , @andreasnoack , @simonbyrne |
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.
If no-one beats me to it, I will merge in a couple of days.
base/random.jl
Outdated
end | ||
|
||
# complex randn | ||
Base.@irrational SQRT_HALF 0.7071067811865475244008 sqrt(big(0.5)) | ||
randn(rng::AbstractRNG, ::Type{Complex{T}}) where {T <: AbstractFloat} = Complex{T}(SQRT_HALF * randn(rng, T), SQRT_HALF*randn(rng, T)) |
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.
inconsistent spacing after SQRT_HALF
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.
Yup. This line is a little long as well. I'll break it at the =
.
base/random.jl
Outdated
@@ -1282,8 +1284,13 @@ let Floats = Union{Float16,Float32,Float64} | |||
$randfun( dims::Integer... ) = $randfun(GLOBAL_RNG, Float64, dims...) | |||
end | |||
end | |||
|
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.
blank line inserted?
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.
oops, not sure how that got there..
Implements sampling from a complex normal distribution by adding a method to
randn
.Extends work done in #17725 by applying some suggestions mentioned there. Fixes #21859.