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

Add complex randn #21973

Merged
merged 5 commits into from
May 25, 2017
Merged

Add complex randn #21973

merged 5 commits into from
May 25, 2017

Conversation

nsmith5
Copy link
Contributor

@nsmith5 nsmith5 commented May 19, 2017

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.

@ararslan ararslan added complex Complex numbers randomness Random number generation and the Random stdlib labels May 19, 2017
@ararslan ararslan requested a review from andreasnoack May 19, 2017 21:11
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))
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

@rfourquet
Copy link
Member

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))
Copy link
Member

@rfourquet rfourquet May 23, 2017

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be AbstractFloat

Copy link
Contributor Author

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

@rfourquet
Copy link
Member

cc @stevengj , @andreasnoack , @simonbyrne

Copy link
Member

@rfourquet rfourquet left a 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))
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line inserted?

Copy link
Contributor Author

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complex Complex numbers randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants