-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix code bug #8
fix code bug #8
Conversation
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.
Looks quite good, once you address these comments it can be merged.
test/runtests.jl
Outdated
W = Float64.([w1 w2]) | ||
H = Float64.([h1; h2]) |
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.
W = Float64.([w1 w2]) | |
H = Float64.([h1; h2]) | |
W = [w1 w2] | |
H = [h1; h2] |
(If you need these, it's probably a code bug that should be fixed.)
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.
This is normalization part W[:, j] = w/normw
, input W is int, but w/normw
is float number, this will through a bug.
test/runtests.jl
Outdated
H_v = [Hn[i, :] for i in axes(Hn, 1)] | ||
|
||
Q1, Q2, _, _, _, _ =build_Qs(W_v, H_v, 1, 2) | ||
@test Q1 == Q1' |
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.
@test Q1 == Q1' | |
@test issymmetric(Q1) |
test/runtests.jl
Outdated
|
||
Q1, Q2, _, _, _, _ =build_Qs(W_v, H_v, 1, 2) | ||
@test Q1 == Q1' | ||
@test Q2 == Q2 |
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.
This is always true?
test/runtests.jl
Outdated
@test Q1 == Q1' | ||
@test Q2 == Q2 | ||
F = eigen(Q1, Q2) | ||
Fvals, Fvecs = F.values::Vector{eltype(Q2)}, F.vectors::Matrix{eltype(Q2)} |
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.
Is this to solve a type-stability issue? That's generally not something to worry about for tests, though of course it doesn't hurt.
But you can solve inference problems in eigen
with Symmetric
(or Hermitian
if you're dealing with complex-valued matrices):
julia> @inferred eigen(Q1, Q2)
ERROR: return type GeneralizedEigen{Float64, Float64, Matrix{Float64}, Vector{Float64}} does not match inferred return type Union{GeneralizedEigen{ComplexF64, ComplexF64, Matrix{ComplexF64}, Vector{ComplexF64}}, GeneralizedEigen{Float64, Float64, Matrix{Float64}, Vector{Float64}}}
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] top-level scope
@ REPL[34]:1
julia> @inferred eigen(Symmetric(Q1), Symmetric(Q2))
GeneralizedEigen{Float64, Float64, Matrix{Float64}, Vector{Float64}}
values:
2-element Vector{Float64}:
4938.9585139612855
84532.04148603871
vectors:
2×2 Matrix{Float64}:
0.968769 -0.768621
-1.19147 -0.331201
test/runtests.jl
Outdated
λ_min = τ/2-sqrt(τ^2/4-δ) | ||
|
||
@test abs(λ_max - maximum(F.values))<=1e-12 | ||
@test abs(λ_min - minimum(F.values))<=1e-12 |
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.
Interestingly, I got this:
julia> @test abs(λ_min - minimum(F.values))<=1e-12
Test Failed at REPL[44]:1
Expression: abs(λ_min - minimum(F.values)) <= 1.0e-12
Evaluated: 4.547473508864641e-12 <= 1.0e-12
Different library versions and CPUs can have slight differences in roundoff error. You might need to make this slightly less stringent. 1e-10, perhaps?
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.
Notice that GitHub actions tests also failed, for the same reason.
test/runtests.jl
Outdated
|
||
w = [u[1], u[2]] | ||
@test norm(u[1].*W_v[1].+u[2].*W_v[2]) ≈ 1 | ||
@test norm(Q1*w - maximum(F.values)*Q2*w) < 1e-12 |
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.
Likewise here I got
julia> @test norm(Q1*w - maximum(F.values)*Q2*w) < 1e-12
Test Failed at REPL[51]:1
Expression: norm(Q1 * w - maximum(F.values) * Q2 * w) < 1.0e-12
Evaluated: 1.4551915228366852e-11 < 1.0e-12
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 cannot got this fail on RIS.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
=======================================
Coverage 81.63% 81.63%
=======================================
Files 1 1
Lines 98 98
=======================================
Hits 80 80
Misses 18 18 ☔ View full report in Codecov by Sentry. |
fix code bug on$\xi$ and $\mathbf{u}$