feat: add `lapack/base/dgetf2` by iampratik13 · Pull Request #12897 · stdlib-js/stdlib

iampratik13 · 2026-06-15T10:16:11Z

Resolves none.

Description

What is the purpose of this pull request?

This pull request:

feat: add lapack/base/dgetf2

Related Issues

Does this pull request have any related issues?

This pull request has the following related issues:

none.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

Read, understood, and followed the contributing guidelines.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

Yes
No

If you answered "yes" above, how did you use AI assistance?

Code generation (e.g., when writing an implementation or fixing a bug)
Test/benchmark generation
Documentation (including examples)
Research and understanding

Disclosure

If you answered "yes" to using AI assistance, please provide a short disclosure indicating how you used AI assistance. This helps reviewers determine how much scrutiny to apply when reviewing your contribution. Example disclosures: "This PR was written primarily by Claude Code." or "I consulted ChatGPT to understand the codebase, but the proposed changes were fully authored manually by myself.".

@stdlib-js/reviewers

stdlib-bot · 2026-06-15T10:19:17Z

Coverage Report

Package	Statements	Branches	Functions	Lines
lapack/base/dgetf2	$\\color{green}383/383$ $\\color{green}+0.00\\%$	$\\color{green}36/36$ $\\color{green}+0.00\\%$	$\\color{green}3/3$ $\\color{green}+0.00\\%$	$\\color{green}383/383$ $\\color{green}+0.00\\%$

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_pkg_readmes status: na - task: lint_markdown_docs status: na - 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 ---

iampratik13 · 2026-06-15T10:49:59Z

https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/lapack/base/dlaswp

kgryte · 2026-06-17T03:27:00Z

+					A[ offsetA + ( j * strideA1 ) + o ] =
+						A[ offsetA + ( jp * strideA1 ) + o ];


Suggested change

A[ offsetA + ( j * strideA1 ) + o ] =

A[ offsetA + ( jp * strideA1 ) + o ];

A[ offsetA + ( j * strideA1 ) + o ] = A[ offsetA + ( jp * strideA1 ) + o ];

Don't line wrap like this. Prefer just disabling the lint rule max-len if it is problematic.

More generally, you can simplify your indexing arithmetic here. You have several repeated calculations. E.g., o + offsetA is a constant. And you use each computed index twice, so storing each in a separate variable is fine, rather than incurring additional arithmetic cost.

kgryte · 2026-06-17T03:29:28Z

+				// Divide A[r, j] by A[j, j] for r = j+1 to M-1
+				temp = A[ offsetA + ( j * strideA1 ) + ( j * strideA2 ) ];
+				for ( r = j + 1; r < M; r++ ) {
+					A[ offsetA + ( r * strideA1 ) + ( j * strideA2 ) ] /= temp;


Same thing. offsetA + ( j * strideA2 ) is a constant. You don't need to perform any multiplication here. You can compute ia = offsetA + ( j*strideA2 ) + ( (j+1)*strideA1 ) and then after computation do ia += strideA1. All the indexing arithmetic you are doing consolidates down to a single in-place addition.

kgryte · 2026-06-17T03:30:16Z

+						A[ offsetA + ( r * strideA1 ) + ( c * strideA2 ) ] -=
+							A[ offsetA + ( r * strideA1 ) + ( j * strideA2 ) ] *
+							temp;


Same general comments. Do not line split. No where in the code base do we do this. And be more efficient in your indexing computations.

kgryte · 2026-06-17T03:34:30Z

+				}
+			}
+		} else if ( info === 0 ) {
+			info = j + 1; // 1-based index


Hmm...this is problematic in the sense that JavaScript doesn't use 1-based indexing, so your explanation of U(k,k) is incorrect in JS. It is a bit unfortunate that 0 is used for success in this case. However, I am not sure we have much of a choice. Instead, you need to explicitly explain in your notes that k equals the status code value MINUS 1.

kgryte · 2026-06-17T03:36:03Z

+	if ( isRowMajor( order ) && LDA < max( 1, N ) ) {
+		throw new RangeError( format( 'invalid argument. Fifth argument must be greater than or equal to max(1,%d). Value: `%d`.', N, LDA ) );
+	}
+	if ( isColumnMajor( order ) ) {
+		sa1 = 1;
+		sa2 = LDA;
+	} else { // order === 'row-major'


Consolidate conditionals. There is no need to first check for a row-major string and then check for a column-major string. At L70, you know that order === 'row-major', so move your LDA validation within the else below.

kgryte · 2026-06-17T03:37:06Z

+	var data = SINGULAR_ROW_MAJOR;
+	var IPIV = new Int32Array( data.IPIV.length );
+	var info;
+	var A = new Float64Array( data.A );


Avoid doing this. This does not follow our style conventions. If you have mixed assignment and declarations, always declare first and then separately assign.

kgryte · 2026-06-17T03:37:53Z

+	t.strictEqual( info, data.info_out, 'returns expected info' );
+	isApprox( t, A, data.A_out, 'returns expected matrix' );
+	checkIPIV( t, IPIV, data.IPIV_out, 'returns expected pivot vector' );


Suggested change

t.strictEqual( info, data.info_out, 'returns expected info' );

isApprox( t, A, data.A_out, 'returns expected matrix' );

checkIPIV( t, IPIV, data.IPIV_out, 'returns expected pivot vector' );

t.strictEqual( info, data.info_out, 'returns expected value' );

isApprox( t, A, data.A_out, 'returns expected value' );

checkIPIV( t, IPIV, data.IPIV_out, 'returns expected value' );

This applies to all of your tests. We are not interested in custom assertion messages, as these will all go away once we move to a new test runner.

kgryte · 2026-06-17T03:38:22Z

+	var A = new Float64Array( 0 );
+
+	info = dgetf2( 'row-major', 0, 2, A, 2, IPIV );
+	t.strictEqual( info, 0, 'returns 0' );


Suggested change

t.strictEqual( info, 0, 'returns 0' );

t.strictEqual( info, 0, 'returns expected value' );

Definitely don't do this where you duplicate the expected value in the test message.

kgryte · 2026-06-17T03:40:10Z

+			delta = abs( actual[ i ] - expected[ i ] );
+			tol = 100.0 * EPS * abs( expected[ i ] );
+			if ( tol === 0.0 ) {
+				tol = 100.0 * EPS;
+			}


Instead of using a tolerance like this, I suggest migrating to using ULP-based accuracy testing. So, use isAlmostSameValue, which should hopefully mitigate the issue with expected[ i ] being zero.

kgryte · 2026-06-17T03:40:24Z

+});
+
+tape( 'the function computes the LU factorization (row-major)', function test( t ) {
+	var data = SQUARE_ROW_MAJOR;


All the same comments.

kgryte · 2026-06-17T03:41:35Z

+-   **LDA**: stride of the first dimension of `A` (leading dimension of the matrix `A` if column-major, or stride between rows if row-major).
+-   **IPIV**: pivot indices as an [`Int32Array`][@stdlib/array/int32]. Pivot indices are 0-based. Should have at least $\min(M, N)$ elements.
+
+Note that indexing is relative to the first index. To introduce an offset, use [`typed array`][mdn-typed-array] views.


You've left out additional examples. Any reason why?

kgryte · 2026-06-17T03:41:43Z

+-   **oa**: starting index of `A`.
+-   **si**: stride length for `IPIV`.
+-   **oi**: starting index of `IPIV`.
+


Same comment.

kgryte

Left initial comments.

feat: add lapack/base/dgetf2

9113c71

iampratik13 requested a review from a team June 15, 2026 10:16

stdlib-bot added LAPACK Issue or pull request related to the Linear Algebra Package (LAPACK). Needs Review A pull request which needs code review. labels Jun 15, 2026

iampratik13 added 2 commits June 15, 2026 15:53

iampratik13 added Feature Issue or pull request for adding a new feature. JavaScript Issue involves or relates to JavaScript. GSoC Google Summer of Code. gsoc: 2026 Google Summer of Code (2026). labels Jun 15, 2026

iampratik13 requested a review from anandkaranubc June 15, 2026 10:45

iampratik13 mentioned this pull request Jun 15, 2026

Weekly Standup (2026-06-15) stdlib-js/meetings#133

Open

kgryte reviewed Jun 17, 2026

View reviewed changes

kgryte requested changes Jun 17, 2026

View reviewed changes

kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Jun 17, 2026

		A[ offsetA + ( j * strideA1 ) + o ] =
		A[ offsetA + ( jp * strideA1 ) + o ];

	A[ offsetA + ( j * strideA1 ) + o ] =
	A[ offsetA + ( jp * strideA1 ) + o ];
	A[ offsetA + ( j * strideA1 ) + o ] = A[ offsetA + ( jp * strideA1 ) + o ];

	t.strictEqual( info, 0, 'returns 0' );
	t.strictEqual( info, 0, 'returns expected value' );

Uh oh!

Conversation

iampratik13 commented Jun 15, 2026

Description

Related Issues

Questions

Other

Checklist

AI Assistance

Disclosure

Uh oh!

stdlib-bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

iampratik13 commented Jun 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgryte left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stdlib-bot commented Jun 15, 2026 •

edited

Loading