algorithm: add float maxvol support - #4
Merged
Conversation
📝 WalkthroughWalkthroughGeneralizes MaxVol and RectMaxVol algorithms from Double-only to a shared Double/Float implementation via a new MaxVolScalar protocol wrapping LAPACK/BLAS routines. Adds public Float overloads, generalizes options resolution, adds Float/numerical test suites, and updates README, ROADMAP, and DocC documentation. ChangesDouble/Float Generalization of MaxVol
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant maxVol
participant maxVolImpl
participant MaxVolScalar
Caller->>maxVol: maxVol(matrix, options)
maxVol->>maxVolImpl: delegate with Scalar type (Double/Float)
maxVolImpl->>MaxVolScalar: getrf (initial pivot rows)
maxVolImpl->>MaxVolScalar: getrs (expansion coefficients)
maxVolImpl->>MaxVolScalar: rankOneUpdate (row swap)
maxVolImpl-->>Caller: MaxVolResult<Scalar>
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/MaxVolTests/FloatMaxVolTests.swift`:
- Around line 263-391: The helper logic for generating reproducible orthonormal
fixtures is duplicated between FloatMaxVolTests and NumericalBehaviorTests;
extract orthonormalColumns, dot, SeededGenerator, and
DenseColumnMajorMatrix<Double>.mapValues into a shared test-support location so
both suites use the same implementation. Move the duplicated definitions into a
common internal test helper (for example, a shared support file under
MaxVolTests) and update the test files to call the shared symbols instead of
maintaining separate copies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7daa57a-b585-4fc2-9163-cac66f6c4447
📒 Files selected for processing (10)
README.mdROADMAP.mdSources/MaxVol/MaxVol.docc/MaxVol.mdSources/MaxVol/MaxVol.docc/ToleranceAndConvergence.mdSources/MaxVol/MaxVol.swiftSources/MaxVol/MaxVolScalar.swiftSources/MaxVol/RectMaxVol.swiftSources/MaxVol/RectMaxVolOptions.swiftTests/MaxVolTests/FloatMaxVolTests.swiftTests/MaxVolTests/NumericalBehaviorTests.swift
Comment on lines
+263
to
+391
| private func expectReconstruction( | ||
| of matrix: DenseColumnMajorMatrix<Float>, | ||
| using result: MaxVolResult<Float>, | ||
| tolerance: Float = 1e-5 | ||
| ) throws { | ||
| for row in 0..<matrix.rows { | ||
| for column in 0..<matrix.columns { | ||
| var reconstructed: Float = 0 | ||
| for selectedColumn in 0..<result.selectedRows.count { | ||
| reconstructed += try result.coefficients.value(row: row, column: selectedColumn) | ||
| * matrix.value(row: result.selectedRows[selectedColumn], column: column) | ||
| } | ||
|
|
||
| let expected = try matrix.value(row: row, column: column) | ||
| #expect(abs(reconstructed - expected) <= tolerance) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func expectCoefficients( | ||
| _ coefficients: DenseColumnMajorMatrix<Float>, | ||
| rowMajorValues: [Float], | ||
| tolerance: Float = 5e-6 | ||
| ) throws { | ||
| let expected = try DenseColumnMajorMatrix( | ||
| rows: coefficients.rows, | ||
| columns: coefficients.columns, | ||
| rowMajorValues: rowMajorValues | ||
| ) | ||
|
|
||
| for row in 0..<coefficients.rows { | ||
| for column in 0..<coefficients.columns { | ||
| let actualValue = try coefficients.value(row: row, column: column) | ||
| let expectedValue = try expected.value(row: row, column: column) | ||
| #expect(abs(actualValue - expectedValue) <= tolerance) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func expectSelectedRowsAreIdentity(_ result: MaxVolResult<Float>) throws { | ||
| for (identityColumn, selectedRow) in result.selectedRows.enumerated() { | ||
| for column in 0..<result.coefficients.columns { | ||
| let expected: Float = column == identityColumn ? 1 : 0 | ||
| let actual = try result.coefficients.value(row: selectedRow, column: column) | ||
| #expect(abs(actual - expected) <= 1e-6) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func maximumAbsoluteCoefficient( | ||
| in coefficients: DenseColumnMajorMatrix<Float> | ||
| ) -> Double { | ||
| coefficients.values.map { Double(abs($0)) }.max() ?? 0 | ||
| } | ||
|
|
||
| private func maximumUnselectedRowNorm(in result: MaxVolResult<Float>) -> Double { | ||
| let selected = Set(result.selectedRows) | ||
| return (0..<result.coefficients.rows) | ||
| .filter { !selected.contains($0) } | ||
| .map { row in | ||
| let normSquared = (0..<result.coefficients.columns).reduce(0.0) { total, column in | ||
| let coefficient = result.coefficients[row: row, column: column] | ||
| return total + Double(coefficient * coefficient) | ||
| } | ||
| return normSquared.squareRoot() | ||
| } | ||
| .max() ?? 0 | ||
| } | ||
|
|
||
| private func orthonormalColumns( | ||
| rows: Int, | ||
| columns: Int, | ||
| seed: UInt64 | ||
| ) throws -> DenseColumnMajorMatrix<Double> { | ||
| var generator = SeededGenerator(state: seed) | ||
| var columnVectors = (0..<columns).map { column -> [Double] in | ||
| (0..<rows).map { row in | ||
| generator.nextDouble() + (row == column ? 1.0 : 0.0) | ||
| } | ||
| } | ||
|
|
||
| for column in 0..<columns { | ||
| for priorColumn in 0..<column { | ||
| let projection = dot(columnVectors[column], columnVectors[priorColumn]) | ||
| for row in 0..<rows { | ||
| columnVectors[column][row] -= projection * columnVectors[priorColumn][row] | ||
| } | ||
| } | ||
|
|
||
| let norm = dot(columnVectors[column], columnVectors[column]).squareRoot() | ||
| for row in 0..<rows { | ||
| columnVectors[column][row] /= norm | ||
| } | ||
| } | ||
|
|
||
| return try DenseColumnMajorMatrix( | ||
| rows: rows, | ||
| columns: columns, | ||
| columnMajorValues: columnVectors.flatMap { $0 } | ||
| ) | ||
| } | ||
|
|
||
| private func dot(_ left: [Double], _ right: [Double]) -> Double { | ||
| zip(left, right).reduce(0) { total, pair in | ||
| total + pair.0 * pair.1 | ||
| } | ||
| } | ||
|
|
||
| private struct SeededGenerator { | ||
| var state: UInt64 | ||
|
|
||
| mutating func nextDouble() -> Double { | ||
| state = state &* 6_364_136_223_846_793_005 &+ 1_442_695_040_888_963_407 | ||
| let scaled = Double(state >> 11) / Double(UInt64.max >> 11) | ||
| return scaled * 2 - 1 | ||
| } | ||
| } | ||
|
|
||
| private extension DenseColumnMajorMatrix where Scalar == Double { | ||
| func mapValues<Output: Sendable>( | ||
| _ transform: (Double) -> Output | ||
| ) throws -> DenseColumnMajorMatrix<Output> { | ||
| try DenseColumnMajorMatrix<Output>( | ||
| rows: rows, | ||
| columns: columns, | ||
| columnMajorValues: values.map(transform) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test helper duplication across FloatMaxVolTests.swift and NumericalBehaviorTests.swift.
orthonormalColumns, dot, SeededGenerator, and the DenseColumnMajorMatrix<Double>.mapValues extension here are byte-for-byte duplicated in Tests/MaxVolTests/NumericalBehaviorTests.swift (lines 156-215). Two independent copies of the PRNG/orthogonalization logic risk silently diverging if one file is updated (e.g., changing the Gram-Schmidt normalization or PRNG constants) without the other, undermining the "reproducible fixture" guarantee both suites rely on.
Consider extracting these into a shared internal test-support file (e.g., Tests/MaxVolTests/TestSupport.swift) that both suites import.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/MaxVolTests/FloatMaxVolTests.swift` around lines 263 - 391, The helper
logic for generating reproducible orthonormal fixtures is duplicated between
FloatMaxVolTests and NumericalBehaviorTests; extract orthonormalColumns, dot,
SeededGenerator, and DenseColumnMajorMatrix<Double>.mapValues into a shared
test-support location so both suites use the same implementation. Move the
duplicated definitions into a common internal test helper (for example, a shared
support file under MaxVolTests) and update the test files to call the shared
symbols instead of maintaining separate copies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
DoubleandFloatmatrices are now supported.Bug Fixes
Documentation
DoubleandFloatusage, options, and algorithm guidance.Tests
Floatresults, randomized matrix cases, and reconstruction accuracy.