Skip to content

inv, size, and \ for QR objects #1300

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gustafsson
Copy link

Fixes #1192

@gustafsson gustafsson force-pushed the 1192-qr-solve branch 2 times, most recently from a654b37 to 354bbd5 Compare March 29, 2025 10:28
Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. I have a few comments.

end
end
push!(ex.args, :(
return SMatrix(R_inv)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use similar_type similarly to _A_ldiv_B above.


@generated function _inv_upper_triangular(R::StaticMatrix{n, n, T}) where {n, T}
ex = quote
R_inv = MMatrix{n,n,T}(undef)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should either use similar (to properly handle non-isbits T) or the same trick as _A_ldiv_B. Also, make sure the method works to T being an integer.

Comment on lines -77 to +84
Base.@propagate_inbounds function invperm(p::StaticVector)
# in difference to base, this does not check if p is a permutation (every value unique)
ip = similar(p)
ip[p] = 1:length(p)
similar_type(p)(ip)
@inline function invperm(p::StaticVector{N,T}) where {N,T<:Integer}
ip = zeros(MVector{N,T})
@inbounds for i in SOneTo(N)
j = p[i]
1 <= j <= N && iszero(ip[j]) || throw(ArgumentError("argument is not a permutation"))
ip[j] = i
end
SVector(ip)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite likely someone relied on this being faster thanks to not performing the checks so I'm not sure about this change.

@@ -62,6 +64,9 @@ function identity_perm(R::StaticMatrix{N,M,T}) where {N,M,T}
return similar_type(R, Int, Size((M,)))(ntuple(x -> x, Val{M}()))
end

is_identity_perm(p::StaticVector{M}) where {M} = all(i->i==p[i], 1:M)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is_identity_perm(p::StaticVector{M}) where {M} = all(i->i==p[i], 1:M)
is_identity_perm(p::StaticVector{M}) where {M} = all(i->i==p[i], SOneTo(M))

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.

Cannot use qr factorization with documented functionalities
2 participants