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

Adjust signatures for bkfact to use Symmetric and Hermitian #22605

Merged
merged 2 commits into from
Jun 30, 2017

Conversation

andreasnoack
Copy link
Member

Factored out of #22601. This is similar to the changes we made to cholfact. The idea is to make Symmetric and Hermitian the default input to the factorization function and that the uplo argument is specified via Symmetric or Hermitian instead of an argument to the factorization function.

@ararslan ararslan added the linear algebra Linear algebra label Jun 29, 2017
end
function bkfact!(A::StridedMatrix, uplo::Symbol, symmetric::Bool = issymmetric(A), rook::Bool = false)
depwarn("bkfact with uplo and symmetric arguments deprecated. Please use bkfact!($(symmetric ? "Symmetric(" : "Hermitian(")A, :$uplo))",
:bkfact)
Copy link
Member

Choose a reason for hiding this comment

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

This and on the line above should be bkfact! rather than bkfact, right?

@tkelman tkelman added deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels Jun 29, 2017
print(io, "successful: $(issuccess(F))")
function Base.show(io::IO, mime::MIME{Symbol("text/plain")}, B::BunchKaufman)
println(io, summary(B))
print(io, "\nsuccessful: $(issuccess(B))")
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to double the newlines?

@@ -48,36 +49,40 @@ bimg = randn(n,2)/2
εb = eps(abs(float(one(eltyb))))
ε = max(εa,εb)

@testset "(Automatic) Bunch-Kaufman factor of indefinite matrix" begin
bc1 = factorize(asym)
# check thactor factorize gives a Bunch-Kaufman
Copy link
Contributor

Choose a reason for hiding this comment

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

check that ?

tkelman
tkelman previously approved these changes Jun 29, 2017
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

lgtm if linalg tests pass on CI
(though a news entry might be worth adding now so we don't have to do it later)

@tkelman tkelman removed the needs news A NEWS entry is required for this change label Jun 29, 2017
end
BunchKaufman(LD, ipiv, char_uplo(uplo), symmetric, rook, info)
BunchKaufman(LD, ipiv, A.uplo, true, rook, info)
end
Copy link
Member

@Sacha0 Sacha0 Jun 29, 2017

Choose a reason for hiding this comment

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

If I read the diff correctly (quite possibly not!), the two preceding bkfact! method bodies are identical? If so, perhaps worth DRYing out? (Edit: Could also reduce the five lines associated with the conditional to the single line LD, ipiv, info = rook ? LAPACK.sytrf_rook!(A.uplo, A.data) : LAPACK.sytrf!(A.uplo, A.data)?)

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. The HermOrSym one should be removed.

LD, ipiv, info = LAPACK.hetrf!(char_uplo(uplo), A)
end
LD, ipiv, info = LAPACK.hetrf!(A.uplo, A.data)
end
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here, if you feel like condensing the five lines into one :). (LD, ipiv, info = rook ? LAPACK.hetrf_rook!(A.uplo, A.data) : LAPACK.hetrf!(A.uplo, A.data).)

bkfact!(convert(Matrix{promote_type(Float32, typeof(sqrt(one(T))))}, A),
uplo, symmetric, rook)
bkfact(A::AbstractMatrix{T}, rook::Bool=false) where {T} =
bkfact!(copy_oftype(A, typeof(sqrt(one(T)))), rook)
Copy link
Member

Choose a reason for hiding this comment

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

Removing the former promotion with Float32 to support e.g. Float16? :)

Copy link
Contributor

@tkelman tkelman Jun 29, 2017

Choose a reason for hiding this comment

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

good eye, missed that change - might want to throw in a test for this bit if there's any behavior that this enables right now

Copy link
Member Author

@andreasnoack andreasnoack Jun 29, 2017

Choose a reason for hiding this comment

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

Hm. I had integer and rational input in mind and then sqrt(one(T)) should be sufficient to get a BLAS type but you are right that a matrix with Float16 would stay Float16. Probably not a big problem, though. If you do arithmetic with Float16 you probably would like to avoid that your elements get promoted silently.

Copy link
Contributor

Choose a reason for hiding this comment

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

so will integer or rational input now behave differently with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. They are unaffected since sqrt(one(T)) isa Float64 for T in (Rational{Int},Int). A Matrix{Float16} will now fail instead of being promoted. I think that is reasonable because people using Float16 most likely prefer an error over silent promotion and I really don't think anybody was ever called bkfact with a Matrix{Float16} in serious code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Float16 is used somewhat frequently in the gpu community. What will the error be in that case? We silently promote for intermediate calculations pretty often and convert back to the destination type when an implementation isn't available in a specific element type.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't do this for factorizations and there hasn't been expressed any demand for this so I think we good for now. We can revisit if Float16 users start asking for this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you answer the question at least?

Copy link
Contributor

Choose a reason for hiding this comment

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

don't all the other factorizations have generic implementations so they wouldn't error?

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll get a MethodError in bkfact!. Not all factorizations have generic fallbacks, e.g. eig and svd.

@tkelman tkelman dismissed their stale review June 29, 2017 23:35

please rebase out the commit from a different pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants