Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 26, 2025

Adds QR decomposition (A = Q·R) factorizing matrices into orthogonal Q and upper triangular R components, following the established pattern of eig/svd/cholesky.

Implementation

  • JVM: LAPACK dgeqrf + dorgqr with workspace queries for optimal performance
  • JS/Native: Placeholder stubs (consistent with other decompositions)
  • Handles rectangular matrices (m×n), returns Q (m×m) and R (m×n)
  • Avoids unnecessary copies, reuses LAPACK-modified arrays where safe

Tests

19 tests covering:

  • Edge cases: single element, single row/column, rank-deficient matrices
  • Dimensions: square, tall (m>n), wide (m<n)
  • Properties: Q orthogonality (Q^T·Q = I), R upper triangular, A = Q·R reconstruction
  • Applications: determinant property, least squares solving
  • Robustness: random matrices, large matrices (20×20)

Usage

import vecxt.all.*

val m = Matrix(Array(1.0, 2.0, 3.0, 4.0), 2, 2)
val (Q, R) = qr(m)  // Q: 2×2 orthogonal, R: 2×2 upper triangular

Updated cheatsheet documentation with examples.

Original prompt

This section details on the original issue you should resolve

<issue_title>QR decomposition: orthogonal + upper-triangular factorization.</issue_title>
<issue_description>Look to things like

https://github.com/Quafadas/vecxt/blob/main/vecxt/src-jvm/eig.scala

for examples on how to structure this. Use the lapack facade on the JVM, and leave a placeholder on native and JS.

Write comprehensive test coverage and make sure that tests on the JVM pass. Try not to copy arrays unecessarily

Review the test coverage for completness and mark that out of 10, make sure the score is above 8 and that the tests pass before asking for a review.

Update the cheatsheet to include a line on how to call the QR decomposition.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits November 26, 2025 17:30
- Add JVM implementation using LAPACK dgeqrf and dorgqr
- Add placeholder for JS/Native platforms
- Create 19 comprehensive tests covering various matrix types
- Update cheatsheet documentation with QR decomposition example
- All tests passing successfully

Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com>
Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com>
Copilot AI changed the title [WIP] Add QR decomposition implementation with test coverage Implement QR decomposition with LAPACK backend Nov 26, 2025
Copilot AI requested a review from Quafadas November 26, 2025 17:34
@Quafadas Quafadas marked this pull request as ready for review November 26, 2025 17:47
Copilot AI review requested due to automatic review settings November 26, 2025 17:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements QR decomposition for the vecxt library, adding the ability to factorize matrices into orthogonal (Q) and upper triangular (R) components following the established pattern of existing decompositions (eigenvalue, SVD, Cholesky).

Key changes:

  • JVM implementation using LAPACK's dgeqrf and dorgqr routines with workspace queries for optimal performance
  • Comprehensive test suite with 19 tests covering edge cases, various dimensions, and mathematical properties
  • Documentation updates including cheatsheet comparison table and working example code

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
vecxt/src-jvm/qr.scala New JVM implementation using LAPACK for QR decomposition with proper error handling and workspace management
vecxt/src-js-native/qr.scala Placeholder stub for JS/Native platforms (consistent with other decomposition stubs)
vecxt/src/all.scala Export QR module to make it available in the main API
vecxt/test/src-jvm/qr.test.scala Comprehensive test suite with 19 tests covering orthogonality, triangularity, reconstruction, and edge cases
site/docs/cheatsheet.md Updated comparison table showing QR decomposition syntax across vecxt, NumPy, and MATLAB
experiments/src/cheatsheet.scala Added example demonstrating QR decomposition usage

Comment on lines +67 to +71
if info.`val` != 0 then throw IllegalStateException(s"QR workspace query failed. INFO=${info.`val`}")
end if

// Allocate workspace
val lwork = math.max(1, workQuery(0).toInt)
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

[nitpick] The workspace query failure handling differs from eig.scala which uses a fallback approach. Consider following the pattern from eig.scala (lines 53-55) which provides a fallback workspace size if the query fails, rather than throwing an exception. This would make the implementation more robust.

Suggested change
if info.`val` != 0 then throw IllegalStateException(s"QR workspace query failed. INFO=${info.`val`}")
end if
// Allocate workspace
val lwork = math.max(1, workQuery(0).toInt)
// If workspace query fails, fallback to a reasonable default (as in eig.scala)
val lwork =
if info.`val` == 0 then math.max(1, workQuery(0).toInt)
else math.max(1, m * n) // fallback: use m*n as workspace size
// Optionally, you could log or comment here: s"QR workspace query failed (INFO=${info.`val`}), using fallback workspace size: $lwork"

Copilot uses AI. Check for mistakes.
Comment on lines +136 to +139
if info.`val` != 0 then throw IllegalStateException(s"Q generation workspace query failed. INFO=${info.`val`}")
end if

val lworkQ = math.max(1, workQuery(0).toInt)
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

[nitpick] The workspace query failure handling differs from eig.scala which uses a fallback approach. Consider following the pattern from eig.scala (lines 53-55) which provides a fallback workspace size if the query fails, rather than throwing an exception. This would make the implementation more robust.

Suggested change
if info.`val` != 0 then throw IllegalStateException(s"Q generation workspace query failed. INFO=${info.`val`}")
end if
val lworkQ = math.max(1, workQuery(0).toInt)
val lworkQ =
if info.`val` == 0 then math.max(1, workQuery(0).toInt)
else
// Fallback: use a reasonable default if workspace query fails (pattern from eig.scala)
math.max(1, m * m)

Copilot uses AI. Check for mistakes.
* - Q: The orthogonal matrix
* - R: The upper triangular matrix
*/
inline def qr(matrix: Matrix[Double]): (Q: Matrix[Double], R: Matrix[Double]) = ???
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

The JS/Native stub signature is missing the BoundsCheck parameter that is present in the JVM implementation. For consistency, the signature should match:

inline def qr(matrix: Matrix[Double])(using inline bc: BoundsCheck): (Q: Matrix[Double], R: Matrix[Double]) = ???

This ensures cross-platform API consistency even though the implementation is a stub.

Suggested change
inline def qr(matrix: Matrix[Double]): (Q: Matrix[Double], R: Matrix[Double]) = ???
inline def qr(matrix: Matrix[Double])(using inline bc: BoundsCheck): (Q: Matrix[Double], R: Matrix[Double]) = ???

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +3
package vecxt

import vecxt.matrix.Matrix
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

Missing import for BoundsCheck type. Add the following import to match the JVM implementation and support the corrected signature:

import vecxt.BoundsCheck.BoundsCheck

Copilot uses AI. Check for mistakes.
@Quafadas Quafadas merged commit 59a4a4d into main Nov 26, 2025
12 checks passed
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.

QR decomposition: orthogonal + upper-triangular factorization.

2 participants