Float16 input now gives Float 16 factors for svd,eigen and cholesky.

[ArunS-tack]

Closes JuliaLang/LinearAlgebra.jl#330.

[dkarrasch]

LGTM. I would write out Float16 instead of using T <: Float16, and make use of conversion functions that already exist. eigen requires a bit more "hacky" work, unfortunately.

[dkarrasch]

For SVD, I think we want to be more permissive and allow both real and complex Float16 eltypes. So you should indeed use the where {T <: Union{Float16,Complex{Float16}} approach, but then use the constructor I recommended.

[ArunS-tack]

would the coverter for eigen go somewhat around these lines?

Eigen{T}(F::Eigen) where T = Eigen( 
     convert(AbstractMatrix{T}, F.vectors), 
     convert(AbstractVector{T}, F.values))

I am probably missing out on something as it is not yet working.

[dkarrasch]

would the coverter for eigen go somewhat around these lines?

Eigen{T}(F::Eigen) where T = Eigen( 
     convert(AbstractMatrix{T}, F.vectors), 
     convert(AbstractVector{T}, F.values))

I am probably missing out on something as it is not yet working.

Yes, but Eigen has more fields: vectorsl, unitary, rconde, rcondv. If these are meaningfully computed by the Float32 computation, you will want to keep that information in the "converted" Eigen{Float16} object. Of course, not all of them need conversion, but some do, I guess: clearly vectorsl, and maybe rconde and rcondv. @KlausC may help here, hopefully.

[ArunS-tack]

I don't see anything about those fields in the documentation but the code surely does have them. An example would be helpful to get an insight.

[ArunS-tack]

Oh, I just checked. rconde, rcondv and vectorsl are outputted as Float16 as is without any further modifications to the methods in here. Added test cases for the same.

[dkarrasch]

Yes, that is true because of how the default values for the optional arguments are set. But now check in all cases (including real symmetric, complex hermitian), if the values of those fields are equal to the values of the Eigen object when you start with a Float32 matrix to begin with. Generally, what I mean is that if these fields are computed (and not set by an empty default), then you want to preserve those values under the conversion, and not "overwrite" them with the default values.

[ArunS-tack]

I see, with default values, it's obvious for them to return as Float16 when given Float16 input 😅. I am still not able to figure out what type of matrix are they calculated for or how to compute them (rcondv,rconde).

[dkarrasch]

Finally, I suggest the following test pattern:

A = ...
B = somefactorization(Float16.(A))
B32 = somefactorization(Float32.(A))
# now the tests that you already have
@test B.somefield ≈ B32.somefield
# for all fields

This should make sure that also the values are as desired, not only the types.

[ArunS-tack]

Ah, I don't think the changes you suggested are working for Real matrix with Complex eigenvalues. rconde and rcondv remain as Float32 when others are converted to Float16 which is why that case probably leads to MethodError. Either we will need to include them in conversion or omit them all together.

[ArunS-tack]

Are we okay with not changing condition numbers to Float16? That is being outputted as Float32.

[dkarrasch]

Are we okay with not changing condition numbers to Float16? That is being outputted as Float32.

Hm, I don't think leaving it as Float32 is harmful, but we could as well convert them to Float16. I wonder if this will lead to many Inf16s, though, because of overflow.

[ArunS-tack]

Are we okay with not changing condition numbers to Float16? That is being outputted as Float32.

Hm, I don't think leaving it as Float32 is harmful, but we could as well convert them to Float16. I wonder if this will lead to many Inf16s, though, because of overflow.

It's working fine at least for now with the examples I have taken. I have converted them to Float16 as well, we could perhaps revert that if someone actually faces the problem you mentioned in the future. Although I don't think anyone will specifically use intense Float16 operations.

[ArunS-tack]

Good to merge? Also I am wondering if we can use conditional operators as you suggested in the eigen method for type converter? That should cover all the cases I guess.