Removing requirement for promotions from 0,1 in lufact #22146
Conversation
|
Thanks for the PR! Could you perhaps also add a test that would fail before this PR? This is good so that this will keep working in the future. |
ararslan
added
linear algebra
base/linalg/generic.jl
Outdated
| @@ -957,7 +957,7 @@ true | |||
| function istriu(A::AbstractMatrix) | |||
| m, n = size(A) | |||
| for j = 1:min(n,m-1), i = j+1:m | |||
| if A[i,j] != 0 | |||
| if A[i,j] != zero(eltype(A)) | |||
There was a problem hiding this comment.
IIUC, !iszero(A[i,j]) should be preferred: It may avoid creation of a zero instance (e.g. for BigInt) and furthermore, there are types T for which zero(T) is not defined although iszero(x::T) is (e.g. matrices).
Applies again below.
There was a problem hiding this comment.
I didn't know about iszero - it doesn't seem to be in the documentation.
But this is good. I will update it to use iszero.
Thanks.
There was a problem hiding this comment.
Its new for the upcoming 0.6 release (#19950) so it is only in the latest docs: https://docs.julialang.org/en/latest/stdlib/numbers.html#Base.iszero
test/linalg/generic.jl
Outdated
| @@ -310,3 +310,33 @@ end | |||
| @test [[1, 2], [3, 4]] ≈ [[1.0-eps(), 2.0+eps()], [3.0+2eps(), 4.0-1e8eps()]] | |||
| @test [[1, 2], [3, 4]] ≉ [[1.0-eps(), 2.0+eps()], [3.0+2eps(), 4.0-1e9eps()]] | |||
| @test [[1,2, [3,4]], 5.0, [6im, [7.0, 8.0]]] ≈ [[1,2, [3,4]], 5.0, [6im, [7.0, 8.0]]] | |||
|
|
|||
| # Issue 22042 | |||
| # Minimal modulo number type - but not subclassing Number | |||
There was a problem hiding this comment.
Perhaps "subtyping" rather than "subclassing"?
There was a problem hiding this comment.
I knew I had the wrong word there! OK.
test/linalg/generic.jl
Outdated
|
|
||
| Base.:+(a::ModInt{n}, b::ModInt{n}) where {n} = ModInt{n}(a.k+b.k) | ||
| Base.:-(a::ModInt{n}, b::ModInt{n}) where {n} = ModInt{n}(a.k-b.k) | ||
| Base.:*(a::ModInt{n}, b::ModInt{n}) where {n} = ModInt{n}(a.k*b.k) |
There was a problem hiding this comment.
Spaces surrounding the operators might make these statements easier on wetware parsers?
There was a problem hiding this comment.
Is this what you mean?
Base.:+(a::ModInt{n}, b::ModInt{n}) where {n} = ModInt{n}(a.k + b.k)
(extra spaces around the infix + )
test/linalg/generic.jl
Outdated
|
|
||
| # Needed for pivoting: | ||
| Base.abs( a::ModInt{n} ) where {n} = a | ||
| Base.:<( a::ModInt{n}, b::ModInt{n} ) where {n} = a.k < b.k |
There was a problem hiding this comment.
Perhaps remove the extraneous spaces surrounding arguments in the method signatures?
test/linalg/generic.jl
Outdated
| b = [ ModInt{2}(1), ModInt{2}(0) ] | ||
|
|
||
| @test A * (A\b) == b | ||
| @test_nowarn lufact( A, Val{true} ) |
test/linalg/generic.jl
Outdated
| # Issue 22042 | ||
| # Minimal modulo number type - but not subclassing Number | ||
| struct ModInt{n} | ||
| k |
There was a problem hiding this comment.
4 space indent is the standard for this repo
| A = [ ModInt{2}(1) ModInt{2}(0) ; ModInt{2}(1) ModInt{2}(1) ] | ||
| b = [ ModInt{2}(1), ModInt{2}(0) ] | ||
|
|
||
| @test A*(A\b) == b |
There was a problem hiding this comment.
This does not actually call lufact (since istril(A) == true), wasn't that the purpose of this PR?
There was a problem hiding this comment.
It is true that I haven't succeed in my original goal, but we are closer. If you make my test matrix not lower triangular, it fails! I think there needs to be a third option for pivoting - where the pivot is chosen by !iszero(..) and not by magnitude. Ultimately I'd like to be able to delete
Base.abs(a::ModInt{n}) where {n} = a
Base.:<(a::ModInt{n}, b::ModInt{n}) where {n} = a.k < b.k
from my test, but this requires a bigger discussion. One suggestion has been to have a symbol as the parameter value instead of the Bool pivot = true or false.
I'm not sure how to proceed - do I open up a new issue or PR?
There was a problem hiding this comment.
| b = [ ModInt{2}(1), ModInt{2}(0) ] | ||
|
|
||
| @test A*(A\b) == b | ||
| @test_nowarn lufact( A, Val{true} ) |
There was a problem hiding this comment.
This errored before this PR. Why test for warning?
There was a problem hiding this comment.
I wasn't sure of the correct way to handle this. It seemed strange to call the function and then return true, since the real test was the function call compiled and returned something successfully.
Issue JuliaLang/LinearAlgebra.jl#431