-
Notifications
You must be signed in to change notification settings - Fork 113
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
unwrap
is not stable when supplied with stable RNG
#616
Comments
Upon review of the unwrap code this afternoon, I determined that this is due to use of |
Possible, but I can't reproduce with julia> measure(s) = ((Random.seed!(10); unwrap(s; dims=1:2)) - (Random.seed!(10); unwrap(s; dims=1:2))).^2 |> mean |> sqrt
measure (generic function with 1 method)
julia> ps = angle.(rand(ComplexF64, 1000, 1000));
julia> measure(ps)
0.0 |
Be sure to start julia with additional threads - |
Yes, that was run on a threaded session. |
It looks like this is down to the semantics of |
I cannot reproduce with the default julia> measure(s, rng, seed) = (unwrap(s; dims=1:2, rng=rng(seed)) - unwrap(s; dims=1:2, rng=rng(seed))).^2 |> mean |> sqrt
measure (generic function with 1 method)
julia> ps = angle.(rand(ComplexF64, 1000, 1000));
julia> measure(ps, StableRNG, 10) == measure(ps, MersenneTwister, 10) == measure(ps, Random.default_rng, 10) == 0
true However, there is indeed a difference with julia> ps = rand(1000, 1000) * 1234;
julia> @test measure(ps, StableRNG, 10) == measure(ps, MersenneTwister, 10) == measure(ps, Random.default_rng, 10) == 0
Test Failed at REPL[11]:1
Expression: measure(ps, StableRNG, 10) == measure(ps, MersenneTwister, 10) == measure(ps, Random.default_rng, 10) == 0
Evaluated: 294.78516174681107 == 160.76991914304145 == 177.74442713272344 == 0
ERROR: There was an error during testing Inspecting the output of different runs of Although yes, this is orthogonal to reproducibility, because even with an invalid input we should maybe get the same invalid output... |
The theory about invalid input is interesting - I was initially going to suggest something about the nature of the random data before I got distracted with the potentially wrong conclusion about task-local RNG. Per the DSP.jl docs,
Anyways, correctness of inputs aside - yes - the behavior should be consistent between applications and means of seeding. |
That inequality looks wrong. Indeed, julia> f(x) = -pi >= x > pi
f (generic function with 1 method)
julia> @code_llvm debuginfo=:none f(100)
; Function Signature: f(Int64)
; Function Attrs: uwtable
define i8 @julia_f_9255(i64 signext %"x::Int64") #0 {
top:
ret i8 0
} Could you try instead all(-pi .< ps .<= pi) # returns false if there is invalid input |
Ah, you are correct. I've been staring at the computer for too long. So, the minimum of the live phase angle data is |
I have been doing some signal analysis and happened to notice when applying unwrap in the below manner that it provides unstable results, even when applied with a stable RNG.
And so forth.
I am trying to measure phase information for comparison between samples, which requires the most (ideally completely) stable measurements as are possible. The error between applications is both significant and well-enough distributed as to propagate to other metrics,
Here is one measurement of a signal:
And another of the same signal - note the visually distinct changes to both the phase heatmap and the troughs at either end of the entropy measurement as well as the fact that the range of the unwrapped data is now (roughly)
[-175, 75]
as opposed to (again, roughly)[-25, 175]
:I am only noticing this fairly late, so have not scrutinized the implementation to see if this is to be expected, is caused by some issue with the wrapped phase information, or is an actual issue. I'll also accept the response that I am using
DSP.unwrap
in an unintended manner, or that I should not expect stability, should it include some better suggestion - I usually work much lower in the stack, where the most complex math tends to be memory alignment (humor attempt complete), so I am still becoming acquainted with some of the stuff here.The text was updated successfully, but these errors were encountered: