Skip to content

Removing requirement for promotions from 0,1 in lufact #22146

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 2, 2017
Merged

Removing requirement for promotions from 0,1 in lufact #22146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f687d50
Removing requirement for promotions from 0,1
arghhhh May 30, 2017
bb3eccf
Using iszero() and added test
arghhhh Jun 1, 2017
4c5038d
Tidy up spaces, tabs etc
arghhhh Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/linalg/generic.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 !iszero(A[i,j])
return false
end
end
Expand Down Expand Up @@ -992,7 +992,7 @@ true
function istril(A::AbstractMatrix)
m, n = size(A)
for j = 2:n, i = 1:min(j-1,m)
if A[i,j] != 0
if !iszero(A[i,j])
return false
end
end
Expand Down
4 changes: 2 additions & 2 deletions base/linalg/lu.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function generic_lufact!(A::StridedMatrix{T}, ::Type{Val{Pivot}} = Val{true}) wh
# find index max
kp = k
if Pivot
amax = real(zero(T))
amax = abs(zero(T))
for i = k:m
absi = abs(A[i,k])
if absi > amax
Expand All @@ -49,7 +49,7 @@ function generic_lufact!(A::StridedMatrix{T}, ::Type{Val{Pivot}} = Val{true}) wh
end
end
ipiv[k] = kp
if A[kp,k] != 0
if !iszero(A[kp,k])
if k != kp
# Interchange
for i = 1:n
Expand Down
30 changes: 30 additions & 0 deletions test/linalg/generic.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 subtyping Number
struct ModInt{n}
k
ModInt{n}(k) where {n} = new(mod(k,n))
end

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)
Base.:-(a::ModInt{n}) where {n} = ModInt{n}(-a.k)
Base.inv(a::ModInt{n}) where {n} = ModInt{n}(invmod(a.k, n))
Base.:/(a::ModInt{n}, b::ModInt{n}) where {n} = a*inv(b)

Base.zero(::Type{ModInt{n}}) where {n} = ModInt{n}(0)
Base.zero(::ModInt{n}) where {n} = ModInt{n}(0)
Base.one(::Type{ModInt{n}}) where {n} = ModInt{n}(1)
Base.one(::ModInt{n}) where {n} = ModInt{n}(1)

# Needed for pivoting:
Base.abs(a::ModInt{n}) where {n} = a
Base.:<(a::ModInt{n}, b::ModInt{n}) where {n} = a.k < b.k
Base.transpose(a::ModInt{n}) where {n} = a # see Issue 20978

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not actually call lufact (since istril(A) == true), wasn't that the purpose of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arghhhh see https://github.com/JuliaLang/julia/issues/22042#issuecomment-305790104

@test_nowarn lufact( A, Val{true} )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This errored before this PR. Why test for warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries :)