LinearAlgebra.givens with unitful arguments

[KlausC]

Solves #36429

[StefanKarpinski]

Thanks! Would be great to add the example from the issue as a test.

[KlausC]

The example is from external package Unitful. Is there a type T in Base with one(T) != oneunit(T)?

[StefanKarpinski]

Right. Sometimes we'll mock up very minimal types for this kind of testing. Not sure if we have one already...

[KlausC]

Right. Sometimes we'll mock up very minimal types for this kind of testing. Not sure if we have one already...

I did not find one. Will make a minimal type similar to Unitful.Quantity in module TestGivens.

[StefanKarpinski]

This seems pretty simple, but it would be good if someone who actually knows what a Givens rotation is took a look. @andreasnoack, could you review?

[dkarrasch]

LGTM, except for one minor style comment.

[dkarrasch]

There is some nasty method ambiguity error, so this cannot be merged as is, unfortunately. It's a strange error, because it seems to relate to multiplication methods defined in the tests, not in the code. Would it be possible to reformulate the MockUnitful tests in terms of Furlongs?

[KlausC]

Would it be possible to reformulate the MockUnitful tests in terms of Furlongs?

Unfortunately not, because Furlong does not have the property that f / oneunit(f) isa Union{AbstractFloat,Complex{<:AbstractFloat}} for f::Furlong, which is needed to call givensAlgorithm.

The fix assumes, that f / oneunit(f) strips off the unit from the type of f; there seems no other way inside Base to accomplish that 👎 . (I could imagine something like stripunit as a companion of oneunit to do that).

The detected ambiguities are an insane interaction of Furlong and MockUnitful. I change the definition of MockUnitful to get rid of it. If they are detected or not seems to be a timing problem, when test-ambiguities is run after test-LinearAlgebra/givens has been executed on the same worker.

[stevengj]

Unfortunately not, because Furlong does not have the property that f / oneunit(f) isa Union{AbstractFloat,Complex{<:AbstractFloat}} for f::Furlong, which is needed to call givensAlgorithm.

Arguably this was a bug in the Furlong test type, which should have been fixed rather than defining yet another unitful test type. It's fixed in #39852.