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

Fix method ambiguity for qr (#931) and lingering ambiguities for lu #932

Merged
merged 3 commits into from
Jul 2, 2021

Conversation

thchr
Copy link
Collaborator

@thchr thchr commented Jul 1, 2021

This basically does just what #921 did to fix #920 for #931; i.e., removes the ambiguity for qr.

While doing that, I noticed that #921 hadn't completely resolved the ambiguity for lu; e.g., if called as lu(A, Val(false)) there would still be an ambiguity error; so I fixed that as well. Similarly, I believe it would previously have been ambiguous if provided with a check keyword argument.

Added a few tests as well.

@thchr
Copy link
Collaborator Author

thchr commented Jul 2, 2021

The failure on nightly is unfortunately related to the strategy for resolving these ambiguities: det doesn't infer because it calls lu(::StaticMatrix, Val{...}; check) which now no longer infers because it @invokes a method with a keyword argument. I.e., this happens:

f(x::Float64; kw=2) = x+kw
f(x::Int;     kw=2) = x+kw

fInt(x; kw=2) = Base.@invoke f(x::Int; kw)
fFloat(x; kw=2) = Base.@invoke f(x::Float64; kw)

where neither fInt nor fFloat infers, despite both methods of f inferring just fine.

So the crux of the problem seems to be that using @invoke to define lu with a check kwarg is bad: but the check kwarg is needed occasionally (e.g. in det apparently).

What would be a good way to sidestep this? Not inferring on lu obviously isn't workable.

@thchr
Copy link
Collaborator Author

thchr commented Jul 2, 2021

The issue with @invoke and keyword arguments was already noted previously in JuliaLang/julia#29216.

@thchr
Copy link
Collaborator Author

thchr commented Jul 2, 2021

Okay, to fix this, I just dropped the @invoke approach entirely and used @eval to instead define methods only for Val{true} and Val{false} pivots, avoiding the need to define the ambiguous Union{Val{true}, Val{false} method at all.
I did this for both qr and lu for consistency, although it's only really necessary for lu.

Locally, all tests pass for me on nightly now.

@mateuszbaran
Copy link
Collaborator

Thanks for investigating it, this approach looks fine. Could you bump the patch version?

@thchr
Copy link
Collaborator Author

thchr commented Jul 2, 2021

Sure: done!

@mateuszbaran mateuszbaran merged commit 7dc6499 into JuliaArrays:master Jul 2, 2021
thchr added a commit to thchr/StaticArrays.jl that referenced this pull request Jul 8, 2021
…ties for `lu` (JuliaArrays#932)

* fix `qr` method ambiguities (JuliaArrays#931) and lingering `lu` ambiguities (JuliaArrays#920)

* fix inferrence issues due to using `@invoke` for `lu` keyword arguments

* bump version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lu seems to be ambiguous on Julia Nightly
2 participants