refactor: update `blas/ext/base/grev` to follow current project conventions #4659

headlessNode · 2025-01-09T05:29:27Z

Description

What is the purpose of this pull request?

This pull request:

update the package to follow current project conventions

Related Issues

Does this pull request have any related issues?

No.

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.

@stdlib-js/reviewers

--- 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: passed - task: lint_javascript_src status: passed - task: lint_javascript_cli status: na - task: lint_javascript_examples status: na - task: lint_javascript_tests status: na - 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: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: na - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

stdlib-bot · 2025-01-09T05:31:11Z

Coverage Report

Package	Statements	Branches	Functions	Lines
blas/ext/base/grev	$\color{green}326/326$ $\color{green}+100.00\%$	$\color{green}22/22$ $\color{green}+100.00\%$	$\color{green}3/3$ $\color{green}+100.00\%$	$\color{green}326/326$ $\color{green}+100.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 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: na - 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: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: passed - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

Planeshifter · 2025-01-11T04:13:21Z

lib/node_modules/@stdlib/blas/ext/base/grev/benchmark/benchmark.js

-	for ( i = 0; i < len; i++ ) {
-		x.push( ( randu()*20.0 ) - 10.0 );
-	}
+	var x = gfillBy( len, new Float64Array( len ), 1, uniform( -100, 100 ) );


Can we please change this back to a generic array instead of Float64Array.

Suggested change

var x = gfillBy( len, new Float64Array( len ), 1, uniform( -100, 100 ) );

var x = gfillBy( len, new Array( len ), 1, uniform( -100, 100 ) );

Same comment.

Planeshifter · 2025-01-11T04:14:09Z

lib/node_modules/@stdlib/blas/ext/base/grev/benchmark/benchmark.ndarray.js

-	for ( i = 0; i < len; i++ ) {
-		x.push( ( randu()*20.0 ) - 10.0 );
-	}
+	var x = gfillBy( len, new Float64Array( len ), 1, uniform( -100, 100 ) );


Suggested change

var x = gfillBy( len, new Float64Array( len ), 1, uniform( -100, 100 ) );

var x = gfillBy( len, new Array( len ), 1, uniform( -100, 100 ) );

@Planeshifter This wasn't the correct change. If we wanted generic arrays, then we should have used @stdlib/array/base/zeros to ensure that the resultant array is not in dictionary mode.

That's problematic, especially for large arrays, as it can tank performance, thus throwing off benchmark results.

@headlessNode Would you mind submitting a follow-up PR addressing this?

This would also be a good lint rule. Namely, we should disallow new Array(...).

@kgryte I actually did use the @stdlib/array/base/zeros, see next commit.

@headlessNode One step ahead of me!

I should have looked at the final code, rather than just relying on the e-mail thread of discussions.

--- 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: na - 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: na - task: lint_typescript_tests status: na - task: lint_license_headers status: passed --- --- type: pre_push_report description: Results of running various checks prior to pushing changes. report: - task: run_javascript_examples status: na - task: run_c_examples status: na - task: run_cpp_examples status: na - task: run_javascript_readme_examples status: na - task: run_c_benchmarks status: na - task: run_cpp_benchmarks status: na - task: run_fortran_benchmarks status: na - task: run_javascript_benchmarks status: passed - task: run_julia_benchmarks status: na - task: run_python_benchmarks status: na - task: run_r_benchmarks status: na - task: run_javascript_tests status: na ---

headlessNode · 2025-01-11T06:45:09Z

@Planeshifter Changes applied.

Planeshifter

Great, thanks!

stdlib-bot · 2025-01-11T14:20:55Z

PR Commit Message

refactor: update `blas/ext/base/grev` to follow current project conventions

PR-URL: https://github.com/stdlib-js/stdlib/pull/4659

Reviewed-by: Philipp Burckhardt <[email protected]>

Please review the above commit message and make any necessary adjustments.

…ntions PR-URL: stdlib-js#4659 Reviewed-by: Philipp Burckhardt <[email protected]>

stdlib-bot added BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). Needs Review A pull request which needs code review. labels Jan 9, 2025

Planeshifter reviewed Jan 11, 2025

View reviewed changes

Planeshifter approved these changes Jan 11, 2025

View reviewed changes

Planeshifter added the Ready To Merge A pull request which is ready to be merged. label Jan 11, 2025

stdlib-bot removed the Needs Review A pull request which needs code review. label Jan 11, 2025

Planeshifter merged commit ea9e425 into stdlib-js:develop Jan 11, 2025
28 checks passed

headlessNode deleted the refactor-grev branch January 12, 2025 10:28

Neerajpathak07 pushed a commit to Neerajpathak07/stdlib that referenced this pull request Jan 23, 2025

refactor: update blas/ext/base/grev to follow current project conve…

13e12dd

…ntions PR-URL: stdlib-js#4659 Reviewed-by: Philipp Burckhardt <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update `blas/ext/base/grev` to follow current project conventions #4659

refactor: update `blas/ext/base/grev` to follow current project conventions #4659

headlessNode commented Jan 9, 2025

stdlib-bot commented Jan 9, 2025 •

edited

Loading

Planeshifter Jan 11, 2025 •

edited

Loading

kgryte Jan 12, 2025

Planeshifter Jan 11, 2025

kgryte Jan 12, 2025

kgryte Jan 12, 2025

kgryte Jan 12, 2025

kgryte Jan 12, 2025

headlessNode Jan 12, 2025

kgryte Jan 12, 2025

kgryte Jan 12, 2025

headlessNode commented Jan 11, 2025

Planeshifter left a comment

stdlib-bot commented Jan 11, 2025

	var x = gfillBy( len, new Float64Array( len ), 1, uniform( -100, 100 ) );
	var x = gfillBy( len, new Array( len ), 1, uniform( -100, 100 ) );

refactor: update blas/ext/base/grev to follow current project conventions #4659

refactor: update blas/ext/base/grev to follow current project conventions #4659

Conversation

headlessNode commented Jan 9, 2025

Description

Related Issues

Questions

Other

Checklist

stdlib-bot commented Jan 9, 2025 • edited Loading

Coverage Report

Planeshifter Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

headlessNode commented Jan 11, 2025

Planeshifter left a comment

Choose a reason for hiding this comment

stdlib-bot commented Jan 11, 2025

PR Commit Message

refactor: update `blas/ext/base/grev` to follow current project conventions #4659

refactor: update `blas/ext/base/grev` to follow current project conventions #4659

stdlib-bot commented Jan 9, 2025 •

edited

Loading

Planeshifter Jan 11, 2025 •

edited

Loading