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

in 1.10+, matrix decomposition components aren't always AbstractMatrix #56106

Open
aplavin opened this issue Oct 11, 2024 · 7 comments
Open

in 1.10+, matrix decomposition components aren't always AbstractMatrix #56106

aplavin opened this issue Oct 11, 2024 · 7 comments
Labels
linear algebra Linear algebra

Comments

@aplavin
Copy link
Contributor

aplavin commented Oct 11, 2024

Tried upgrading Julia version from 1.9 in one of my envs, and noticed this:

julia> using LinearAlgebra

julia> f(a::AbstractMatrix) = sum(a)  # MWE

julia> f(qr(rand(5,5)).Q)
# 1.9:
-1.4572588703587335
# 1.10+:
ERROR: MethodError: no method matching f(::LinearAlgebra.QRCompactWYQ{Float64, Matrix{Float64}, Matrix{Float64}})

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:

However, upgrading to the next Stable release will always be possible as each release of Julia v1.x will continue to run code written for earlier versions.

Introduced in #46196 – deliberately, it wasn't just an oversight.

@aplavin aplavin changed the title in 1.10, matrix decomposition components aren't always AbstractMatrix in 1.10+, matrix decomposition components aren't always AbstractMatrix Oct 11, 2024
@giordano
Copy link
Contributor

https://docs.julialang.org/en/v1.9/stdlib/LinearAlgebra/#LinearAlgebra.qr

Q matrix can be converted into a regular matrix with Matrix.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 11, 2024

Could you elaborate a bit? Of course, I need Matrix() to convert an AbstractMatrix to a regular Matrix. But how is that related here?

@giordano
Copy link
Contributor

Could you elaborate a bit?

The documentation of Julia v1.9 is saying that you if you want to convert Q to a regular matrix you should use Matrix(Q).

But how is that related here?

It's related because that was the documented way to go already in Julia v1.9.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 11, 2024

convert Q to a regular matrix you should use Matrix(Q)

Note that Q was never a Matrix, and I don't expect it.
But I do expect to be able to pass Q to functions that take AbstractMatrices.

@giordano
Copy link
Contributor

You may be a victim of Hyrum's Law, where you relied on pre-existing unintended and undocumented behaviour.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 11, 2024

you relied on pre-existing unintended and undocumented behaviour

What exactly is not documented? That "returns a matrix" means "AbstractMatrix"?
This is very common in Julia docs, both in terms of return values and argument types. Just clicked on the very first function on the linear algebra page:

tr(M)
Matrix trace. Sums the diagonal elements of M.

Naturally, tr() is defined for AbstractMatrix.
The same is often for "array" in docs = "AbstractArray", both in Julia and in packages.

This is a documented and expected behavior, the linked PR even broke several existing packages. Note that code relying on qr().Q being an AbstractMatrix is most likely to appear in the enduser code, not in packages: one reasonably expects to take a function from some package that works on "matrices" (AbstractMatrix) and apply it to a QR decomposition component.
Actually, LinearAlgebra.tr(qr(A).Q) doesn't work – no need to even go to packages.

If that's not a breaking change, very few things are :)
If such changes are expected in the future as well, should we clarify the stability docs then?

upgrading to the next Stable release will always be possible as each release of Julia v1.x will continue to run code written for earlier versions. * ** *** ****
*: if the code is in a registered package
**: if the relevant codepath is covered by tests
***: if you make sure your tests always pass on nightly
****: if Julia devs don't decide that breaking your code is a minor change and ask you to change it

@mbauman
Copy link
Sponsor Member

mbauman commented Oct 11, 2024

@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 f(Q::AbstractQ) = f(Matrix(Q)). And it would be great to update QR's docs to ensure it doesn't talk about Q as a "matrix".

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.

@mbauman mbauman added the linear algebra Linear algebra label Oct 11, 2024
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

No branches or pull requests

3 participants