Add svd for real Symmetric and Hermitian matrices

[dkarrasch]

Closes JuliaLang/LinearAlgebra.jl#628.

[StefanKarpinski]

If we could get a quick review from some committer with linalg expertise, this seems straightforward enough and it would be nice to get it merged.

[dkarrasch]

Oh, if it's really urgent, I think I can help here 😄 . I ended up using exactly @stevengj 's suggestion JuliaLang/LinearAlgebra.jl#628 after the discussion in JuliaLang/LinearAlgebra.jl#628. Should have mentioned that earlier.

And yes, since svd(::Hermitian) errors on all 1.x versions, but this PR does not add a completely new functionality, I'd say this could be backported...?

[stevengj]

It's a new feature, not a bugfix, so it shouldn't be backported.

[StefanKarpinski]

but this PR does not add a completely new functionality

I can see how it would seem that way since we already had SVD and Symmetric and Hermitian matrix types. However, adding previously unimplemented methods is adding new functionality. The general way to think about it is that patch releases should be both forward and backwards compatible with each other except for bugs: i.e. if your application works with Julia version x.y.z₁ then it should work with x.y.z₂ and vice versa unless there is a bug in one of those versions that your application specifically hits. Backporting this would violate that since someone could write code for Julia 1.1.2 that uses these svd methods which would not work on Julia 1.1.0 or 1.1.1 (which is about to be released).

[dkarrasch]

I see, thanks @StefanKarpinski for the explanation!

[StefanKarpinski]

@stevengj, could you review?

[stevengj]

Also seems like this should be outside the for (a, a2) loop since it only uses asym

[dkarrasch]

Good catch! Actually, that loop is mysterious. a2 is never used in it. I should still move the test outside, but should I also simplify that loop? Or leave it until someone guesses what a2 was meant to be good for and includes it in tests?

[stevengj]

Simplifying that loop would be good.

[stevengj]

While you're at it, you could probably also merge the adjoint and transpose tests with for transform in (adjoint, transpose)

[dkarrasch]

There are a couple of other unused variables. I wonder about the following: after setting Random.seed!(1234321), and then calling

areal = randn(n,n)/2
aimg  = randn(n,n)/2
a2real = randn(n,n)/2 # unused
a2img  = randn(n,n)/2 # unused

the latter two are not used, anywhere. If I remove them, does that change the random numbers created in subsequent rand* calls? Probably yes. But is that an issue? Should I leave them for test consistency? I don't think that we rely here on that the random numbers in future tests are exactly the same as they used to be, but I'm not sure how strictly that is handled.

[StefanKarpinski]

Should I leave them for test consistency?

No, it's ok to change them, but if tests start failing it may require using a different seed or loosening some approximate equality tests. The main reason for using a fixed seed is so that we don't get random failures during CI, but if the bounds are good then most seeds should be fine.

[dkarrasch]

Tests pass locally.