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

Avoid copy in getindex(::AbstractQ, ...) #44729

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

dlfivefifty
Copy link
Contributor

It's possible (and reasonable) that a subtype of AbstractQ overloads copy to return itself. This change avoids an infinite recursion that would result in this situation.

@jishnub
Copy link
Contributor

jishnub commented Mar 24, 2022

Probably should be backported to v1.8

@dkarrasch dkarrasch added domain:linear algebra Linear algebra backport 1.8 Change should be backported to release-1.8 labels Mar 25, 2022
@dkarrasch
Copy link
Member

Should we have some test with a little dummy struct? For example (fails on master with the "desired" stack overflow):

struct MyIdentity{T} <: LinearAlgebra.AbstractQ{T} end
Base.size(::MyIdentity, dim::Integer) = dim in (1,2) ? 2 : 1
Base.copy(J::MyIdentity) = J
LinearAlgebra.lmul!(::MyIdentity{T}, M::Array{T}) where {T} = M
@test MyIdentity{Float64}()[1,:] == [1.0, 0.0]

@dkarrasch
Copy link
Member

While developing the test proposal, I noticed another strange thing, a few lines above the changed code:

size(Q::AbstractQ, dim::Integer) = size(getfield(Q, :factors), dim == 2 ? 1 : dim)

But we don't expect (or do we?) any AbstractQ subtype to have a field factors, so this should perhaps be restricted to the subtypes provided by LinearAlgebra. Downstream packages will need to define their own size, which they likely do anyways. Perhaps, this should be

size(Q::Union{HessenbergQ, QRCompactWYQ, QRPackedQ}, dim::Integer) = size(getfield(Q, :factors), dim == 2 ? 1 : dim)

The missing type in this list, from the sysimg, is QRSparseQ, which already has

https://github.com/JuliaSparse/SuiteSparse.jl/blob/f63732c1c6adecb277d8f2981cc8c1883c321bcc/src/spqr.jl#L136

@dkarrasch
Copy link
Member

Since this is meant for backport, I'll merge this as is and do the size change on master to avoid any confusion.

@dkarrasch dkarrasch merged commit ea82910 into JuliaLang:master Mar 25, 2022
KristofferC pushed a commit that referenced this pull request Mar 28, 2022
@KristofferC KristofferC mentioned this pull request Mar 28, 2022
22 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants