fix: revert uniform implementation and remove unnecessary loop #5314

abhishekblue · 2025-02-19T09:31:23Z

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

Resolves #5228

Description

What is the purpose of this pull request?

This pull request:

Reverts @stdlib/random/base/uniform back to @stdlib/random/array/uniform as per review feedback in fb90f93#r152597788.
Updates benchmark.native.js to match the expected implementation.
Removes unnecessary for loop and directly initializes the uniform array.

Related Issues

Does this pull request have any related issues?

This pull request:

Address commit comments (commit fb90f93).

Questions

Any questions for reviewers?

No.

Other

Any other relevant information?

make lint produced filename errors in unrelated files. These files were not modified as part of this PR.
Skipping lint fixes since they are unrelated to this issue.

Checklist

Read, understood, and followed the [contributing guidelines]: https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md

@stdlib-js/reviewers

--- 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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

stdlib-bot · 2025-02-19T09:32:55Z

Coverage Report

Package	Statements	Branches	Functions	Lines
stats/base/dists/planck/median	$\color{green}163/163$ $\color{green}+100.00\%$	$\color{green}8/8$ $\color{green}+100.00\%$	$\color{green}2/2$ $\color{green}+100.00\%$	$\color{green}163/163$ $\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

Neerajpathak07 · 2025-02-19T11:44:38Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.native.js

-		lambda[ i ] = uniform( 1.0, 10.0 );
-	}
+
+	lambda = uniform(len, 0.1, 10.0);


Suggested change

lambda = uniform(len, 0.1, 10.0);

lambda = uniform( 100, 0.1, 10.0 );

as per the suggested changes the var len should be removed as the original implementation worked just fine.
Could you pls do the same for the benchmark.js file as well.

Just adding on to it, once the len variable is removed, the initialization of y in the benchmark loop will be updated to:

y = median( lambda[ i % lambda.length ] );

Basically, everything that was done in the original PR, as Neeraj already pointed out.

Neerajpathak07 · 2025-02-19T11:44:58Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

 var bench = require( '@stdlib/bench' );
-var Float64Array = require( '@stdlib/array/float64' );
+var uniform = require( '@stdlib/random/array/uniform' );
 var uniform = require( '@stdlib/random/base/uniform' );


Suggested change

var uniform = require( '@stdlib/random/base/uniform' );

this should be removed as per the original implementation you will only be needing @stdlib/random/array/uniform.

@anandkaranubc what's your take on this?

I agree. The latter one needs to be removed since it is no longer in use. Thanks for pointing that out!

there is lint error in line no 39 , 42

Yes, I believe clicking on the details should provide an overview for it.

anandkaranubc · 2025-02-19T13:02:25Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

 var bench = require( '@stdlib/bench' );
-var Float64Array = require( '@stdlib/array/float64' );
+var uniform = require( '@stdlib/random/array/uniform' );
 var uniform = require( '@stdlib/random/base/uniform' );


I agree. The latter one needs to be removed since it is no longer in use. Thanks for pointing that out!

anandkaranubc · 2025-02-19T13:04:45Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.native.js

-		lambda[ i ] = uniform( 1.0, 10.0 );
-	}
+
+	lambda = uniform(len, 0.1, 10.0);


Just adding on to it, once the len variable is removed, the initialization of y in the benchmark loop will be updated to:

y = median( lambda[ i % lambda.length ] );

Basically, everything that was done in the original PR, as Neeraj already pointed out.

abhishekblue · 2025-02-19T15:25:42Z

Hi @Neerajpathak07 , my PR is ready, and I have made all the requested changes. Could you please approve the workflows? Thanks!

Neerajpathak07 · 2025-02-19T17:14:06Z

@abhishekblue have you committed and pushed those changes cause they don't reflect here??
And the workflow approvals lie with the maintainers.

--- 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 ---

abhishekblue · 2025-02-20T12:44:38Z

Hi @Neerajpathak07,
Apologies for the confusion earlier! I had committed the changes but mistakenly thought they were already pushed.
I have now pushed the latest changes. Could you please check if they reflect correctly and approve the workflows if everything looks good?
I also have to remove for loop from benchmark.js as well.
Thanks for your patience!

Neerajpathak07 · 2025-02-20T13:02:58Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

 	len =100;
 	lambda = new Float64Array( len );


Suggested change

len =100;

lambda = new Float64Array( len );

won't be needing the Float64Array module so better to remove it

Neerajpathak07 · 2025-02-20T13:03:11Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

-		lambda[ i ] = uniform( 1.0, 10.0);
-	}
+
+	lambda = uniform(100, 1.0, 10.0); 


Suggested change

lambda = uniform(100, 1.0, 10.0);

lambda = uniform( 100, 0.1, 10.0 );

Need the same change for the native version as well.

Neerajpathak07 · 2025-02-20T13:04:52Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.native.js

 	var y;
 	var i;

 	len = 100;


here as well!

You'll also want to remove

var uniform = require( '@stdlib/random/base/uniform' );

and add

var uniform = require( '@stdlib/random/array/uniform' );

Neerajpathak07 · 2025-02-20T13:08:58Z

@abhishekblue Just a few changes rest everything looks in line to me. Thank you for your efforts to correct them!!

--- 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 ---

abhishekblue · 2025-02-20T13:19:01Z

@Neerajpathak07 I've made the suggested formatting changes and also removed unused variables. Let me know if anything else is required.
Thanks!

--- 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 ---

Neerajpathak07 · 2025-02-20T13:35:08Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

-	for ( i = 0; i < len; i++ ) {
-		lambda[ i ] = uniform( 1.0, 10.0);
-	}
+


Suggested change

please remove this extra line.
Same for the native file.

--- 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 ---

anandkaranubc · 2025-02-20T16:09:06Z

lib/node_modules/@stdlib/stats/base/dists/planck/median/benchmark/benchmark.js

-	for ( i = 0; i < len; i++ ) {
-		lambda[ i ] = uniform( 1.0, 10.0);
-	}
+	lambda = uniform( 100, 1.0, 10.0 ); 


Suggested change

lambda = uniform( 100, 1.0, 10.0 );

lambda = uniform( 100, 0.1, 10.0 );

Something that doesn’t make a huge difference but would be better to have the same range as the original for consistency. :)

absolutely! had suggested something similar in this suggestion must have been overlooked.

--- 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 ---

Neerajpathak07 · 2025-02-20T16:44:14Z

LGTM!!
@kgryte Kindly take a look at this and merge it eventually.

Planeshifter · 2025-02-22T02:28:09Z

Thanks @abhishekblue for your PR and to @saurabhraghuvanshii, @Neerajpathak07 and @anandkaranubc for review comments!

stdlib-bot · 2025-02-22T02:29:58Z

PR Commit Message

bench: revert uniform implementation and remove unnecessary loop

PR-URL: https://github.com/stdlib-js/stdlib/pull/5314
Closes: https://github.com/stdlib-js/stdlib/issues/5228

Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Neeraj Pathak <[email protected]>
Reviewed-by: Karan Anand <[email protected]>
Reviewed-by: Saurabh Singh <[email protected]>

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

stdlib-bot added Statistics Issue or pull request related to statistical functionality. First-time Contributor A pull request from a contributor who has never previously committed to the project repository. Needs Review A pull request which needs code review. labels Feb 19, 2025

stdlib-bot reviewed Feb 19, 2025

View reviewed changes

Neerajpathak07 reviewed Feb 19, 2025

View reviewed changes

anandkaranubc requested changes Feb 19, 2025

View reviewed changes

anandkaranubc removed the Needs Review A pull request which needs code review. label Feb 19, 2025

kgryte added the Needs Changes Pull request which needs changes before being merged. label Feb 20, 2025

Neerajpathak07 reviewed Feb 20, 2025

View reviewed changes

anandkaranubc requested changes Feb 20, 2025

View reviewed changes

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

stdlib-bot removed the Needs Changes Pull request which needs changes before being merged. label Feb 22, 2025

chore: remove trailing space

d70146b

Planeshifter approved these changes Feb 22, 2025

View reviewed changes

Planeshifter merged commit 576bd4b into stdlib-js:develop Feb 22, 2025
10 checks passed

abhishekblue mentioned this pull request Mar 31, 2025

[RFC]: Automated Code Reviews and Fixes via LLM-powered stdlib-bot stdlib-js/google-summer-of-code#124

Closed

7 tasks

	lambda = uniform(len, 0.1, 10.0);
	lambda = uniform( 100, 0.1, 10.0 );

	lambda = uniform(100, 1.0, 10.0);
	lambda = uniform( 100, 0.1, 10.0 );

	lambda = uniform( 100, 1.0, 10.0 );
	lambda = uniform( 100, 0.1, 10.0 );

Uh oh!

fix: revert uniform implementation and remove unnecessary loop #5314

fix: revert uniform implementation and remove unnecessary loop #5314

Uh oh!

Conversation

abhishekblue commented Feb 19, 2025

Resolves #5228

Description

Related Issues

Questions

Other

Checklist

Uh oh!

stdlib-bot left a comment

Choose a reason for hiding this comment

Uh oh!

stdlib-bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neerajpathak07 Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

abhishekblue commented Feb 19, 2025

Uh oh!

Neerajpathak07 commented Feb 19, 2025

Uh oh!

abhishekblue commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neerajpathak07 Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Neerajpathak07 commented Feb 20, 2025

Uh oh!

abhishekblue commented Feb 20, 2025

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!

Neerajpathak07 commented Feb 20, 2025

Uh oh!

Planeshifter commented Feb 22, 2025

Uh oh!

stdlib-bot commented Feb 22, 2025 • edited by Planeshifter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Commit Message

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

stdlib-bot commented Feb 19, 2025 •

edited

Loading

Neerajpathak07 Feb 19, 2025 •

edited

Loading

Neerajpathak07 Feb 20, 2025 •

edited

Loading

stdlib-bot commented Feb 22, 2025 •

edited by Planeshifter

Loading