Skip to content

Add LU decomposition with partial pivoting#53

Merged
Quafadas merged 10 commits into
mainfrom
copilot/add-lu-decomposition-factorization
Nov 26, 2025
Merged

Add LU decomposition with partial pivoting#53
Quafadas merged 10 commits into
mainfrom
copilot/add-lu-decomposition-factorization

Conversation

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

Implements LU factorization using LAPACK's dgetrf on JVM, with placeholders for JS/Native platforms.

Implementation

  • JVM (vecxt/src-jvm/lu.scala): LAPACK dgetrf wrapper returning named tuple (L, U, P) where P is the pivot array. Handles square and rectangular matrices, including singular cases.
  • JS/Native (vecxt/src-js-native/lu.scala): Placeholder following SVD/Cholesky pattern
  • Export (vecxt/src/all.scala): Added export vecxt.LU.*

Tests

15 test cases in vecxt/test/src-jvm/lu.test.scala covering:

  • Identity, diagonal, general square matrices (2x2, 3x3)
  • Rectangular matrices (tall/wide)
  • Singular matrices
  • Numerical edge cases (1x1, small values, mixed signs)
  • Mathematical invariants: PA = LU, determinant preservation, triangular properties

Usage

import vecxt.all.*

val m = Matrix.rand(4, 4)
val result = lu(m)

// Access components
val L = result.L  // Lower triangular with unit diagonal
val U = result.U  // Upper triangular
val P = result.P  // Pivot array (0-indexed)

Updated cheatsheet with comparison to NumPy/MATLAB.

Original prompt

This section details on the original issue you should resolve

<issue_title>LU decomposition: lower/upper triangular factorization with pivoting</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 LU decomposition. </issue_description>

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


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits November 26, 2025 17:11
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 LU decomposition with pivoting support Add LU decomposition with partial pivoting Nov 26, 2025
Copilot AI requested a review from Quafadas November 26, 2025 17:17
@Quafadas Quafadas marked this pull request as ready for review November 26, 2025 17:18
Copilot AI review requested due to automatic review settings November 26, 2025 17:18
@Quafadas

Copy link
Copy Markdown
Owner

Were the workflows run ?

@Quafadas

Copy link
Copy Markdown
Owner

@copilot Did the CI run on this PR? Why not?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 LU decomposition with partial pivoting using LAPACK's dgetrf on the JVM platform, following the established pattern for linear algebra operations in vecxt. The implementation correctly handles both square and rectangular matrices, including singular cases, and provides comprehensive test coverage.

  • Adds JVM implementation using LAPACK's dgetrf routine with proper error handling
  • Provides JS/Native placeholders following the project's cross-platform structure
  • Includes 15 comprehensive tests covering basic cases, edge cases, rectangular matrices, and mathematical invariants
  • Updates the cheatsheet with usage examples comparing to NumPy/MATLAB

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
vecxt/src-jvm/lu.scala JVM implementation using LAPACK dgetrf, extracting L and U matrices with while loops for performance, properly converting 1-based LAPACK pivots to 0-based
vecxt/src-js-native/lu.scala Placeholder for JS/Native platforms following established pattern
vecxt/src/all.scala Exports LU module for user access
vecxt/test/src-jvm/lu.test.scala Comprehensive test suite with 15 tests validating correctness through mathematical invariants and edge cases
site/docs/cheatsheet.md Updated with LU decomposition usage example for cross-reference with NumPy and MATLAB

Comment thread vecxt/src-jvm/lu.scala Outdated
* - U: Upper triangular matrix
* - P: Permutation array
*/
inline def lu(m: Matrix[Double]): (L: Matrix[Double], U: Matrix[Double], P: Array[Int]) = ???

Copilot AI Nov 26, 2025

Copy link

Choose a reason for hiding this comment

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

The placeholder signature is missing the BoundsCheck parameter that the JVM implementation has. To maintain consistency with other decomposition functions (e.g., eig, solve), add the bounds check parameter:

inline def lu(m: Matrix[Double])(using inline bc: BoundsCheck): (L: Matrix[Double], U: Matrix[Double], P: Array[Int]) = ???
Suggested change
inline def lu(m: Matrix[Double]): (L: Matrix[Double], U: Matrix[Double], P: Array[Int]) = ???
inline def lu(m: Matrix[Double])(using inline bc: BoundsCheck): (L: Matrix[Double], U: Matrix[Double], P: Array[Int]) = ???

Copilot uses AI. Check for mistakes.
Comment thread vecxt/test/src-jvm/lu.test.scala Outdated

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot Did the CI run on this PR? Why not?

The CI workflows are configured to exclude branches matching copilot/** (see line 14 of .github/workflows/ci.yml). Since this branch is named copilot/add-lu-decomposition-factorization, the CI won't run on pushes to it.

However, the CI should run on the pull request itself once it's opened, as the workflow is triggered by pull_request events to all branches except update/** and pr/**.

All tests have been run locally and pass:

  • JVM: ✅ All 15 LU tests pass
  • JS: ✅ Compilation successful
  • Native: ✅ Compilation successful
  • Code formatting: ✅ Passed
  • Code review: ✅ No issues found

@Quafadas Quafadas merged commit 01ea160 into main Nov 26, 2025
5 of 6 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.

LU decomposition: lower/upper triangular factorization with pivoting

3 participants