feat: add lapack/base/dlasv2#2819
Open
Pranavchiku wants to merge 17 commits into
Open
Conversation
3 tasks
Pranavchiku
commented
Sep 2, 2024
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: passed
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
Contributor
|
@kgryte I have made some changes and would like to know your thoughts on the same. |
kgryte
reviewed
May 12, 2026
| * | ||
| * @private | ||
| */ | ||
| function main() { |
Member
There was a problem hiding this comment.
@prajjwalbajpai This benchmarks needs to be refactored. The min/max iteration is pointless as the benchmark is not parameterized. We should use the unparameterized version of benchmarks.
kgryte
reviewed
May 12, 2026
| * @private | ||
| * @returns {Function} benchmark function | ||
| */ | ||
| function createBenchmark( ) { |
Member
There was a problem hiding this comment.
@prajjwalbajpai Same thing. This benchmark is incorrect.
kgryte
reviewed
May 12, 2026
| * | ||
| * var out = new Float64Array( 6 ); | ||
| * | ||
| * out = dlasv2( 2.0, 3.0, 4.0, out ); |
Member
There was a problem hiding this comment.
Suggested change
| * out = dlasv2( 2.0, 3.0, 4.0, out ); | |
| * dlasv2( 2.0, 3.0, 4.0, out ); |
No need to overwrite out here. Applies here and throughout this PR.
kgryte
reviewed
May 12, 2026
| * var out = new Float64Array( 6 ); | ||
| * | ||
| * out = dlasv2( 2.0, 3.0, 4.0, out ); | ||
| * // out => <Float64Array>[ 1.5513263285176897, 5.1568776039816795, 0.9664996487646696, 0.25666793515702424, 0.7496781758158659, 0.6618025632357402 ] |
Member
There was a problem hiding this comment.
We should be using approximate values (e.g., ~1.5513), rather than full-precision, in order to guard against implementation drift invalidating all documentation. Applies here and throughout this PR.
Member
There was a problem hiding this comment.
@prajjwalbajpai This still needs to be updated to use approximate values. E.g.,
// out => <Float64Array>[ ~1.5513, ~4.1569, ... ]This comment applies here and everywhere else.
kgryte
reviewed
May 12, 2026
kgryte
reviewed
May 12, 2026
| > var out = new {{alias:@stdlib/array/float64}}( 6 ); | ||
| > {{alias}}( 2.0, 3.0, 4.0, out ); | ||
| > out[ 0 ] | ||
| 1.5513263285176897 |
Member
There was a problem hiding this comment.
Same thing. Use approximate values.
kgryte
reviewed
May 12, 2026
Comment on lines
+51
to
+52
| buffer, the offset parameters support indexing semantics based on starting | ||
| indices. |
Member
There was a problem hiding this comment.
Suggested change
| buffer, the offset parameters support indexing semantics based on starting | |
| indices. | |
| buffer, the offset parameter support indexing semantics based on a starting | |
| index. |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
| expected = new Float64Array( [ 1.0, PINF, 0.0, 1.0, 0.0, 1.0 ] ); | ||
| out = new Float64Array( 6 ); | ||
| out = dlasv2( PINF, 2.0, 1.0, out ); | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
|
|
||
| dlasv2( data.F, data.G, data.H, out ); | ||
| expected = data.out; | ||
| isApprox( t, out, expected, 1.0 ); |
Member
There was a problem hiding this comment.
Suggested change
| isApprox( t, out, expected, 1.0 ); | |
| isApprox( t, out, expected, 1 ); |
kgryte
reviewed
May 15, 2026
| * @param {Object} t - test object | ||
| * @param {Collection} actual - actual values | ||
| * @param {Collection} expected - expected values | ||
| * @param {number} rtol - relative tolerance |
Member
There was a problem hiding this comment.
All the same changes as proposed for the other test file.
kgryte
reviewed
May 15, 2026
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (Negative Stride)', function test( t ) { |
Member
There was a problem hiding this comment.
Suggested change
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (Negative Stride)', function test( t ) { | |
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (negative stride)', function test( t ) { |
Applies here and below.
kgryte
reviewed
May 15, 2026
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (Offset)', function test( t ) { |
Member
There was a problem hiding this comment.
Suggested change
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (Offset)', function test( t ) { | |
| tape( 'the function computes the singular value decomposition of a triangular matrix with elements in increasing order (offset)', function test( t ) { |
Applies here and below.
kgryte
reviewed
May 15, 2026
|
|
||
| <section class="intro"> | ||
|
|
||
| The singular value decomposition of a 2x2 upper triangular matrix $ \begin{bmatrix} F & G \\ 0 & H \end{bmatrix}$ is given by - |
Member
There was a problem hiding this comment.
Suggested change
| The singular value decomposition of a 2x2 upper triangular matrix $ \begin{bmatrix} F & G \\ 0 & H \end{bmatrix}$ is given by - | |
| The singular value decomposition of a 2x2 upper triangular matrix $ \begin{bmatrix} F & G \\ 0 & H \end{bmatrix}$ is given by |
kgryte
requested changes
May 15, 2026
Member
kgryte
left a comment
There was a problem hiding this comment.
Left another round of comments. This is coming along.
@prajjwalbajpai Thank you for improving the tests. They are muuuuuch better. 🙌
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Contributor
|
Made the changes. |
Co-authored-by: Athan <kgryte@gmail.com> Signed-off-by: Athan <kgryte@gmail.com>
kgryte
reviewed
May 15, 2026
Co-authored-by: Athan <kgryte@gmail.com> Signed-off-by: Athan <kgryte@gmail.com>
kgryte
reviewed
May 15, 2026
| * @param {Collection} expected - expected values | ||
| * @param {number} maxULPs - maximum allowed ULP difference | ||
| */ | ||
| function isApprox( t, actual, expected, maxULPs ) { |
Member
There was a problem hiding this comment.
I just recalled that this function is not necessary. You can replace all usage of isApprox below with @stdlib/assert/is-almost-same-float64array. Applies to this and the other test file.
Contributor
There was a problem hiding this comment.
Done. One thing I found interesting was:
If I directly create the expected array as
expected = data.out;The isAlmostSameValueFloat64Array function was returning false. Hence, I had to wrap the array in Float64Array.
expected = new Float64Array( data.out );This is something that is not done in other routines where fixtures are used.
aayush0325
requested changes
May 15, 2026
Member
aayush0325
left a comment
There was a problem hiding this comment.
looks fine to me, please check the latex rendering issue!
| var Float64Array = require( '@stdlib/array/float64' ); | ||
| var dlasv2 = require( './../lib/' ); | ||
|
|
||
| var out = new Float64Array( [ 0.0, 0.0, 0.0, 0.0, 0.0, 0.0 ] ); |
Member
There was a problem hiding this comment.
Suggested change
| var out = new Float64Array( [ 0.0, 0.0, 0.0, 0.0, 0.0, 0.0 ] ); | |
| var out = new Float64Array( 6 ); |
Comment on lines
+226
to
+232
| out[ offsetOut ] = ssmin; | ||
| out[ offsetOut + strideOut ] = ssmax; | ||
| out[ offsetOut + ( 2 * strideOut ) ] = snr; | ||
| out[ offsetOut + ( 3 * strideOut ) ] = csr; | ||
| out[ offsetOut + ( 4 * strideOut ) ] = snl; | ||
| out[ offsetOut + ( 5 * strideOut ) ] = csl; | ||
| return out; |
Member
There was a problem hiding this comment.
the index calculation can be done in fewer ops here
|
|
||
| <section class="intro"> | ||
|
|
||
| The singular value decomposition of a 2x2 upper triangular matrix $ \begin{bmatrix} F & G \\ 0 & H \end{bmatrix}$ is given by |
Member
There was a problem hiding this comment.
is it just me or is the latex rendering messed up for you guys as well @kgryte @prajjwalbajpai
Member
There was a problem hiding this comment.
it would make sense to document how you're testing using fortran in a separate repository, like this repo this would help us both in terms of reviews and be a source of truth as more LAPACK PRs get queued for review over the next few weeks. imo its a good starting point, then we can consider keeping the fortran files itself in the repo but im not sure if that would achieve anything
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
aayush0325
reviewed
May 15, 2026
|
|
||
| <!-- </equation> --> | ||
|
|
||
| where `CSL`,`SNL`,`CSR`,`SNR` define orthogonal rotations and $\mathrm{SSMAX}^2, \mathrm{SSMIN}^2$ are the eigenvalues of $A^{T}A$. |
Member
There was a problem hiding this comment.
what is A here? assuming its the 2x2 input matrix you can write that above
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
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
5 participants
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.
Towards #2464.
Description
This pull request adds JS implementation for
lapack/base/dlasv2Related Issues
NA
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers