Skip to content

Add docstring to the rank(::QRPivoted) method #1365

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

Merged

Conversation

Luis-Varona
Copy link
Contributor

@Luis-Varona Luis-Varona commented May 29, 2025

With the release of Julia 1.12, LinearAlgebra.jl is adding dispatches for rank to SVD and QRPivoted objects, allowing the re-use of existing matrix factorizations where they have already been computed. The SVD method already has an appropriate docstring; this PR adds a near-identical one to the QRPivoted method as well, with some additional context for how it differs from the default SVD-based computation.

For reference, the existing docstring for the SVD method is:

"""
    rank(S::SVD{<:Any, T}; atol::Real=0, rtol::Real=min(n,m)*ϵ) where {T}

Compute the numerical rank of the SVD object `S` by counting how many singular values are greater 
than `max(atol, rtol*σ₁)` where `σ₁` is the largest calculated singular value.
This is equivalent to the default [`rank(::AbstractMatrix)`](@ref) method except that it re-uses an existing SVD factorization.
`atol` and `rtol` are the absolute and relative tolerances, respectively.
The default relative tolerance is `n*ϵ`, where `n` is the size of the smallest dimension of UΣV'
and `ϵ` is the [`eps`](@ref) of the element type of `S`.

!!! compat "Julia 1.12"
    The `rank(::SVD)` method requires at least Julia 1.12.
"""

The proposed docstring for the QRPivoted method follows a very similar structure, with some added context (given that QR, unlike SVD, is not the default factorization for computing rank):

"""
    rank(A::QRPivoted{<:Any, T}; atol::Real=0, rtol::Real=min(n,m)*ϵ) where {T}

Compute the numerical rank of the QR factorization `A` by counting how many diagonal entries of
`A.factors` are greater than `max(atol, rtol*Δ₁)` where `Δ₁` is the largest calculated such entry.
This is similar to the [`rank(::AbstractMatrix)`](@ref) method insofar as it counts the number of
(numerically) nonzero coefficients from a matrix factorization, although the default method uses an
SVD instead of a QR factorization. Like [`rank(::SVD)`](@ref), this method also re-uses an existing
matrix factorization.

Using a QR factorization to compute rank should typically produce the same result as using SVD,
although it may be more prone to overestimating the rank in pathological cases where the matrix is
ill-conditioned. It is also worth noting that it is generally faster to compute a QR factorization
than it is to compute an SVD, so this method may be preferred when performance is a concern.
`atol` and `rtol` are the absolute and relative tolerances, respectively.

The default relative tolerance is `n*ϵ`, where `n` is the size of the smallest dimension of `A`
and `ϵ` is the [`eps`](@ref) of the element type of `A`.

!!! compat "Julia 1.12"
    The `rank(::QRPivoted)` method requires at least Julia 1.12.
"""

@Luis-Varona Luis-Varona force-pushed the rank-qrpivoted-documentation branch from d909ee3 to 23aaf7e Compare May 29, 2025 02:51
Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.81%. Comparing base (b265fea) to head (5da8050).
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1365   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files          34       34           
  Lines       15717    15717           
=======================================
  Hits        14745    14745           
  Misses        972      972           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ViralBShah ViralBShah added the backport 1.12 Change should be backported to release-1.12 label May 30, 2025
@ViralBShah
Copy link
Member

This looks good to me.

@Luis-Varona Luis-Varona force-pushed the rank-qrpivoted-documentation branch 2 times, most recently from 5cc0adc to 9265f60 Compare June 4, 2025 13:35
@Luis-Varona Luis-Varona requested a review from stevengj June 4, 2025 13:35
@Luis-Varona
Copy link
Contributor Author

@stevengj, I'm done editing the docstring as requested (hopefully it's all okay now). Thanks for the feedback!

By the way—I opened issue stevengj/nlopt#610 a few months ago, and I was wondering if you could take a look at it if you still happen to maintain that repository 🙂

@jishnub jishnub mentioned this pull request Jun 5, 2025
4 tasks
@Luis-Varona
Copy link
Contributor Author

@jishnub @stevengj Is there anything else you'd like me to change in the docstring, or does it seem all good? 🙂

With the release of Julia 1.12, LinearAlgebra.jl is adding dispatches for rank to SVD and QRPivoted objects, allowing the re-use of existing matrix factorizations where they have already been computed. The SVD method already has an appropriate docstring; this PR adds a near-identical one to the QRPivoted method as well, with some additional context for how it differs from the default SVD-based computation.
@Luis-Varona Luis-Varona force-pushed the rank-qrpivoted-documentation branch from 9265f60 to 5da8050 Compare June 8, 2025 03:00
@Luis-Varona
Copy link
Contributor Author

@stevengj, I fixed that one inaccuracy in the docstring you pointed out. Is there anything else you'd like me to change?

@Luis-Varona Luis-Varona requested a review from stevengj June 8, 2025 03:06
@Luis-Varona
Copy link
Contributor Author

Just following up, @stevengj. 🙂

jishnub added a commit that referenced this pull request Jun 13, 2025
Backported PRs:
- [x] #1364 <!-- clarify eigen docstring -->
- [x] #1363 <!-- Change default symmetriceigen algorithm back to
RobustRepresentations -->

Non-merged PRs with backport label:
- [ ] #1365 <!-- Add docstring to the `rank(::QRPivoted)` method -->
- [ ] #1305 <!-- Bounds-checking in triangular indexing branches -->
@ViralBShah ViralBShah merged commit c5d7122 into JuliaLang:master Jun 18, 2025
4 checks passed
@Luis-Varona Luis-Varona deleted the rank-qrpivoted-documentation branch June 18, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants