-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
in 1.10+, matrix decomposition components aren't always AbstractMatrix and don't behave like them #1097
Comments
https://docs.julialang.org/en/v1.9/stdlib/LinearAlgebra/#LinearAlgebra.qr
|
Could you elaborate a bit? Of course, I need |
The documentation of Julia v1.9 is saying that you if you want to convert
It's related because that was the documented way to go already in Julia v1.9. |
Note that |
You may be a victim of Hyrum's Law, where you relied on pre-existing unintended and undocumented behaviour. |
What exactly is not documented? That "returns a matrix" means "AbstractMatrix"?
Naturally, This is a documented and expected behavior, the linked PR even broke several existing packages. Note that code relying on If that's not a breaking change, very few things are :)
|
@aplavin What were the actual methods you needed and found to be missing? Were they in LinearAlgebra or in your package? I think the best path forward here is to define those as This was identified as a breaking change right from the get-go and its pros were very deliberately weighed against the estimated costs of the breakage. It's totally fair that you might weigh those pros and cons differently. |
From a cursory look, I saw the immediate error happen in three cases:
Most likely, there are more downstream functions that also require AbstractMatrix, didn't check that.
Sounds like "Julia minor versions are non-breaking, you just need to update your code and it will continue working" :) I believe setting the expectations correctly is important: Julia advertises itself as backwards-compatible, with package compatibility additionally checked by PkgEval. In practice, even when breakage is caught by PkgEval (ie far from always), it can easily be dismissed as a minor change. |
Perhaps it's better to distinguish between julia ( |
Of course, if LinearAlgebra wasn't shipped with the base Julia install, and had a breaking change in a minor version, I would've opened the issue in their repo. IME this is quite rare for packages: many tend to lean the other way and release major versions even for almost-not-breaking changes, actually following semver. |
I take the blame. Yes, this was a breaking change, no doubt. Just to recap, what was done/considered to justify it:
All of that has nothing to do with semver and doesn't justify formally breaking it, so I plead guilty. But I hope you agree that the "justifying" considerations have heavy weight. If I can help with anything, let me know. |
This kind of code is much more likely to be found in end-user code, not in packages. A package defining
Sure, removing subtyping generally reduces the number of methods applicable, and is expected to improve latency. But isn't this kind of dispatch one of the major Julia selling points?
A lot of matrix decompositions aren't done for performance, and even slow but correct generic implementations are completely fine in those cases. All the above points motivate There are so many ways to steer performance-sensitive QR decomposition users to the new implementation without breaking the existing code!
|
Noticed yet another example of perfectly reasonable calculation that should work, and worked without any special effort in 1.9-, but got broken by this julia> using LinearAlgebra, StructArrays
julia> qr(rand(3,3)).Q * StructArray([1+2im, 3, 4])
# 1.9-:
3-element StructArray(::Vector{Float64}, ::Vector{Float64}) with eltype ComplexF64:
-4.8444364108512055 - 0.9826988305649018im
1.0784776480572889 - 1.0628267141640224im
1.1697528900840188 - 1.3801095550954214im
# 1.10+:
ERROR: MethodError: no method matching lmul!(::LinearAlgebra.QRCompactWYQ{ComplexF64, Matrix{…}, Matrix{…}}, ::StructVector{ComplexF64, @NamedTuple{…}, Int64}) |
This is clearly a bug that needs to be patched |
This is an interesting case. The reason why this fails on v1.10 is because we don't have a generic |
Tried upgrading Julia version from 1.9 in one of my envs, and noticed this:
qr(A).Q
not being an AbstractMatrix is very surprising even in isolation, but especially so given the previous behavior in Julia.How come it's allowed in Julia 1.x? Seems like the clear and unambiguous definition of breaking change...
From Julia docs:
Introduced in JuliaLang/julia#46196 – deliberately, it wasn't just an oversight.
The text was updated successfully, but these errors were encountered: