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

LinearAlgebra.givens with unitful arguments #36430

Merged
merged 7 commits into from
Jul 3, 2020
Merged

LinearAlgebra.givens with unitful arguments #36430

merged 7 commits into from
Jul 3, 2020

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Jun 25, 2020

Solves #36429

@StefanKarpinski
Copy link
Member

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

@KlausC
Copy link
Contributor Author

KlausC commented Jun 25, 2020

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

@StefanKarpinski
Copy link
Member

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

@KlausC
Copy link
Contributor Author

KlausC commented Jun 25, 2020

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.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Jun 25, 2020
@StefanKarpinski
Copy link
Member

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?

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

LGTM, except for one minor style comment.

stdlib/LinearAlgebra/src/givens.jl Outdated Show resolved Hide resolved
Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

KlausC commented Jul 2, 2020

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.

@dkarrasch dkarrasch merged commit b79a6d1 into JuliaLang:master Jul 3, 2020
@KlausC KlausC deleted the krc/givensunitful branch July 3, 2020 10:57
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@stevengj
Copy link
Member

stevengj commented Mar 1, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants