-
-
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
lufact for sparse matrix pivot option error #18246
Conversation
base/sparse/umfpack.jl includes the following method definition for lufact `lufact(A::SparseMatrixCSC, pivot::Type{Val{false}}) = lufact(A)` This should likely be `lufact(A::SparseMatrixCSC, pivot::Type{Val{true}}) = lufact(A)` because in lufact pivoting is on by default. The error is shown in the following example ``` A = speye(4) A[1:2,1:2] = [-.01 -200; 200 .001] F = lufact(A,Val{false}) F[:p] ``` which returns ``` julia> F[:q] 4-element Array{Int64,1}: 3 4 1 2 ``` However it should return ``` julia> F[:q] 4-element Array{Int64,1}: 1 2 3 4 ``` because pivoting was turned off.
lufact for sparse matrix pivot option error
Thanks. That method is indeed wrong. I'm wondering why we have it. @jiahao Do you remember why you added it? |
I don't recall... Ref: #18244 @garrettthomaskth Could you help us add the nice test case you show as a test in |
#18246,18244-lufact sparse pivot | ||
let | ||
A = speye(4) |
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.
4 space indent
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. okay added the 4 space indent
oops my bad
@garrettthomaskth Could you try just to remove the method and see what happens? Since we don't support turning off pivoting I don't think this method is necessary. |
Great. Thanks. |
why should deleting a method be backported? |
The method was wrong. It pretended to disable pivoting but didn't. |
* lufact for sparse matrix pivot option error base/sparse/umfpack.jl includes the following method definition for lufact `lufact(A::SparseMatrixCSC, pivot::Type{Val{false}}) = lufact(A)` This should likely be `lufact(A::SparseMatrixCSC, pivot::Type{Val{true}}) = lufact(A)` because in lufact pivoting is on by default. The error is shown in the following example ``` A = speye(4) A[1:2,1:2] = [-.01 -200; 200 .001] F = lufact(A,Val{false}) F[:p] ``` which returns ``` julia> F[:q] 4-element Array{Int64,1}: 3 4 1 2 ``` However it should return ``` julia> F[:q] 4-element Array{Int64,1}: 1 2 3 4 ``` because pivoting was turned off. * Added test for JuliaLang#18246 and JuliaLang#18244 * 4 space indent oops my bad * remove unnecessary lufact method * update test for removed lufact method definition
* lufact for sparse matrix pivot option error base/sparse/umfpack.jl includes the following method definition for lufact `lufact(A::SparseMatrixCSC, pivot::Type{Val{false}}) = lufact(A)` This should likely be `lufact(A::SparseMatrixCSC, pivot::Type{Val{true}}) = lufact(A)` because in lufact pivoting is on by default. The error is shown in the following example ``` A = speye(4) A[1:2,1:2] = [-.01 -200; 200 .001] F = lufact(A,Val{false}) F[:p] ``` which returns ``` julia> F[:q] 4-element Array{Int64,1}: 3 4 1 2 ``` However it should return ``` julia> F[:q] 4-element Array{Int64,1}: 1 2 3 4 ``` because pivoting was turned off. * Added test for #18246 and #18244 * 4 space indent oops my bad * remove unnecessary lufact method * update test for removed lufact method definition (cherry picked from commit 68d3d32)
base/sparse/umfpack.jl includes the following method definition for lufact
lufact(A::SparseMatrixCSC, pivot::Type{Val{false}}) = lufact(A)
which should be
lufact(A::SparseMatrixCSC, pivot::Type{Val{true}}) = lufact(A)
because in lufact pivoting is on by default.
The error is shown in the following example
which returns
However it should return
because pivoting was turned off.