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 kron
ambiguity for triangular/hermitian matrices
#54413
Conversation
There are some problems. First is that you removed the restriction to a subtype of m = UpperTriangular(reshape(fill(ones(2,2),4), 2, 2))
n = Symmetric(reshape(fill(ones(2,2),4), 2, 2))
kron(m,m)
kron(n,n) The second is that you removed the parametrization of the functions. I don't understand why, we really want the compiler to specialize on The third is that now if we wrap |
Hm, I see. Dispatching on type parameters is just not recommended (especially when we have so many existing methods already), since it's prone to introducing ambiguities and increases latency.
IIRC, then this is what the compiler does: it specializes on function arguments whenever they are called in the function body, and not merely passed on as an argument. I'll need to double-check.
Sure, but now we have stdlibs in an ambiguous state. So we can't |
It's better than introducing a bug, though. If you can fix the underlying bugs to make
Well they are just passed on as an argument, no? I'm pretty sure it wasn't specializing without it.
Sure, I was just complaining, we can't let a method ambiguity stay. |
Not to insist, but to learn something: I read https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing as it will specialize since in our case, the |
Fair enough, it is specializing then. I did a quick test to confirm it: f(x) = x^2
g(x) = abs2(x)
test(z::Real) = do_power(z,f)
test(z::Complex) = do_power(z,g)
do_power(z,h) = h(z) It wasn't actually me who added this parametrization, but I can't find in the PR who did, so I could ask what they had in mind. I guess it doesn't really matter. |
Could well be me (who suggested it) at the time. I remember looking it up not so long ago. |
How about this now? The signature could be relaxed again once somebody writes sparse kron-kernels, but for now I think this is not too bad. |
I think it's fine now. I'd suggest adding the four lines I posted above to the tests, so that this bug doesn't get accidentally reintroduced. |
Thanks! I've opened an issue to track the underlying problem. |
This resolves method ambiguities in SparseArrays.jl, that have been introduced in #53186. As usual, we would want the result to be sparse if any one Kronecker factor is sparse. In that case, however, we would need a specialized kernel to begin with. So, this is more like a quick fix.
This has not been released, so it's not a regression.
@araujoms Is this fine with you?