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

Condition number is computed wrong in some cases #33547

Closed
goggle opened this issue Oct 13, 2019 · 3 comments · Fixed by #33548
Closed

Condition number is computed wrong in some cases #33547

goggle opened this issue Oct 13, 2019 · 3 comments · Fixed by #33548
Labels

Comments

@goggle
Copy link
Contributor

goggle commented Oct 13, 2019

I have discovered this issue while working on another one:

julia> M = [1.0 -2.0; -2.0 -1.5]
2×2 Array{Float64,2}:
  1.0  -2.0
 -2.0  -1.5

julia> S = Symmetric(M)
2×2 Symmetric{Float64,Array{Float64,2}}:
  1.0  -2.0
 -2.0  -1.5

julia> cond(M, 1)
1.909090909090909

julia> cond(S, 1)
2.227272727272727

These two condition numbers should be the same, but they are clearly not.

@ViralBShah ViralBShah added the domain:linear algebra Linear algebra label Oct 13, 2019
@antoine-levitt
Copy link
Contributor

The issue is that _cond1Inf in lu.jl calls gecon, which is only an estimate of the matrix norm, so S is actually the correct one. Let's just remove the specialization and just compute opnorm(A, p)*opnorm(inv(A), p) in all cases? Condition number is only a diagnostic tool, so its speed is not that important. Affects #33297.

@goggle
Copy link
Contributor Author

goggle commented Oct 13, 2019

@antoine-levitt Yes exactly, I was preparing a PR for #33297, when this issue appeared after writing some tests for it.

Yes, I think removing the specialization

_cond1Inf(A::StridedMatrix{<:BlasFloat}, p::Real)

would resolve this.

@antoine-levitt
Copy link
Contributor

I went ahead and did a PR for this, if you have any additional fixes or tests feel free to add them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants