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

Consistency of return type for matrix functions #12408

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

mfasi
Copy link
Contributor

@mfasi mfasi commented Jul 31, 2015

I added specialized versions of expm, logm and sqrtm for symmetric and Hermitian matrices.

EDIT: The documentation is now in #12432

@KristofferC
Copy link
Member

This fails the whitespace checker.

@mfasi mfasi force-pushed the matfun_special_matrices branch 2 times, most recently from 5df6507 to d4a5850 Compare July 31, 2015 14:39
@kshyatt kshyatt added docs This change adds or pertains to documentation linear algebra Linear algebra labels Jul 31, 2015
@mfasi
Copy link
Contributor Author

mfasi commented Aug 1, 2015

I made the templates clearer, as suggested by @jiahao, and added a bunch of tests to cover a few missing lines in dense.jl.

@andreasnoack
Copy link
Member

I think the convertions with full are right when the input is a StridedMatrix. That is also how we do it in inv.

Still if a matrix is either Hermitian or Symmetric, then f(A) will be Symmetric for sure

I think this is only correct for real elements. E.g.

julia> A = [1 im; -im 1.0]      
2x2 Array{Complex{Float64},2}:  
 1.0+0.0im  0.0+1.0im           
 0.0-1.0im  1.0+0.0im         

julia> ishermitian(A)        
true                         

julia> expm(A) |> t -> norm(t - t.')      
6.38905609893065                            

I'm not an expert on matrix functions, but we probably want for keep separate defition loops for entire and non-entire functions. I guess we can ensure type stability for entire functions with Symmetric and Hermitian arguments whereas this is not the the case for the logarithm and root functions.

I forgot to mention that it's a great update of the documentation.


.. function:: logm(A)

Compute the matrix logarithm of ``A``.
If ``A`` has no negative real eigenvalue, compute the principal matrix logarithm of ``A``, i.e. the unique matrix :math:`X` such that :math:`e^X = A` and :math:`-\pi < Im(\lambda) < \pi` for all the eigenvalues :math:`\lambda` of :math:`X`. If ``A`` has nonpositive eigenvalues, a warning is printed and whenever possible a nonprimary matrix function is returned.
Copy link
Member

Choose a reason for hiding this comment

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

possible a nonprimary matrix function is returned

Should it be "non-principal"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreasnoack You're right, thank you for spotting this, I'll fix it and submit a separate PR just for the documentation (I'll have to adapt it to #11943).

@mfasi
Copy link
Contributor Author

mfasi commented Aug 2, 2015

The documentation in now in #12432.

@mfasi mfasi changed the title Consistency of return type and documentation for matrix functions Consistency of return type for matrix functions Aug 2, 2015
@mfasi
Copy link
Contributor Author

mfasi commented Aug 3, 2015

@andreasnoack You're right, that condition is true only for real Hermitian, that is Symmetric. On the other hand, I think that you'd be right in real arithmetic, but numerically you cannot ensure that expm(A) is Hermitian by just knowing that A is Hermitian. An example that shows this (unless you're very lucky):

A1 = randn(4,4) + im*randn(4,4); A2 = A1' + A1; ishermitian(A2)

If you check diag(expm(A2)) you'll see that the imaginary part of the elements on the diagonal is small but not 0, so numerically the matrix is not Hermitian.

EDIT: it looks like this can happen even if A2 is positive definite, for example

A1 = randn(4,4) + im*randn(4,4); A2 = A1' + A1; ishermitian(A2)

which quite settles the question, I think. If A is Symmetric, so is f(A). If it is Hermitian, the result can be of any type, so returning a StridedMatrix seems appropriate.

@andreasnoack
Copy link
Member

@mfasi Part of the point in having the Hermitian and Symmetric types is circumvent this numerical noice problem and exploit what we know mathematically, i.e. that expm(Hermitian) is Hermitian. We are doing an exact check of the imaginary part of the diagonal on construction which would cause an error in the example you present, but I thinks we should then simply set the imaginary parts to zero.

@mfasi
Copy link
Contributor Author

mfasi commented Aug 5, 2015

@andreasnoack I submitted a version that enforces the return type when the perturbation of the Hermitian structure is due to rounding errors. As the exponential is the only entire function we have, I didn't implement a loop for it.

if isposdef(F)
retmat = F.vectors * Diagonal(($func)(F.values)) * F.vectors'
if T <: Real
return real(Hermitian(retmat))
Copy link
Member

Choose a reason for hiding this comment

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

Why is real necessary here?

@andreasnoack
Copy link
Member

@mfasi Is this ready? I don't think I have more comment. Would you do a git rebase origin/master and forced push then we might be lucky enough to get AppVeyor running the tests.

@mfasi
Copy link
Contributor Author

mfasi commented Aug 10, 2015

@andreasnoack Yes, I thought it was ready. I rebased on the master branch, but now the apple build on Travis fails as well. Not sure what happened, since I cannot access the whole log file, but I'm pretty sure the problem is not related to what I changed, it looks like it's trying to compile some fortran library. I'll try to rebase and force again as soon as a new commit is merged to the master branch.

…atrices, fix type returned by logm and sqrtm
@mfasi
Copy link
Contributor Author

mfasi commented Aug 10, 2015

Both Travis and AppVeyor managed to build and check, feel free to merge.

andreasnoack added a commit that referenced this pull request Aug 10, 2015
Consistency of return type for matrix functions
@andreasnoack andreasnoack merged commit 79604f2 into JuliaLang:master Aug 10, 2015
@andreasnoack
Copy link
Member

Great. Thanks.

@mfasi mfasi deleted the matfun_special_matrices branch August 10, 2015 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants