bench: refactor random number generation in `stats/base/dists/kumaraswamy` by yuvimittal · Pull Request #5019 · stdlib-js/stdlib

yuvimittal · 2025-02-02T09:43:44Z

Resolves #4975

Description

What is the purpose of this pull request?

This pull request:

refactor random number generation in JS benchmarks

Related Issues

Does this pull request have any related issues?

This pull request:

resolves [RFC]: Refactor random number generation in JS benchmarks for stats/base/dists/kumaraswamy #4975

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

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

Neerajpathak07 · 2025-02-02T16:42:51Z

@yuvi-mittal
Hey, the native file should be migrated as well right?

yuvimittal · 2025-02-02T16:59:57Z

@Neerajpathak07 , seems like just the benchmarks .

Neerajpathak07 · 2025-02-02T17:14:59Z

Ohh just had a look at the package and apparently there are no C implementations for it so in turn there is no native file got it!
But wouldn't it be right that when we start implementing the packages serial wise we can refactor the the logic for both the benchmark files in one single go!!.
Increases the clarity of the PR and might help while reviewing it.
What's your opinion on this?

yuvimittal · 2025-02-03T07:35:54Z

There C implementation is not done yet , so some of them doesnt have the native benchmarks .

Neerajpathak07 · 2025-02-03T07:51:20Z

And primarily you have test failures for stats/base/dists/kumaraswamy/median/test/test.native this file solely.
So, might be something to do with the main.js implementations or the tol is pretty low for this package.
Let me take a look at the specific package and get back to you

yuvimittal · 2025-02-03T07:56:12Z

i didnt edit stats/base/dists/kumaraswamy/median/test/test.native , But i will check and resolve the error

Neerajpathak07 · 2025-02-03T08:08:53Z

i didnt edit stats/base/dists/kumaraswamy/median/test/test.native , But i will check and resolve the error

Alright I understood where the problem lies, so the stats/base/dists/kumaraswamy/median/test/test.native.js isn't correct with respect to test.js as multiple test cases are missing from native version which are present in test.js.
So, the test.native.js will be needing a little refactoring before we can proceed with this.

Neerajpathak07 · 2025-02-03T08:11:46Z

For reference take a look at test.js and test.native.js and comapre between the two.

Neerajpathak07 · 2025-02-03T08:16:09Z

@kgryte What are your views on this?
Like I do the think there was something wrong in the native implementation for the kumaraswamy/median package as majority of the test cases are failing due to extremely low tolerance.

yuvimittal · 2025-02-03T10:31:54Z

Both test.js and test.native.js need to be refactored . I will open an issue for this and raise a separate PR . But how did it get merged in the first place ?

Neerajpathak07 · 2025-02-03T13:19:25Z

You can draft this PR for the time being

anandkaranubc · 2025-02-03T02:55:46Z

+		dist.a = y[i % len];
+		if ( dist.a !== y[i % len] ) {


Suggested change

dist.a = y[i % len];

if ( dist.a !== y[i % len] ) {

dist.a = y[ i % len ];

if ( dist.a !== y[ i % len ] ) {

anandkaranubc · 2025-02-03T02:57:27Z

+		dist.b = y[i % len];
+		if ( dist.b !== y[i % len] ) {


Same change as above.

anandkaranubc · 2025-02-03T03:01:40Z

+		x[ i ] = uniform( EPS, 2.0 );
+		a[ i ] = uniform( EPS, 100.0 );
+		s[ i ] = uniform( EPS, 100.0 );


Suggested change

x[ i ] = uniform( EPS, 2.0 );

a[ i ] = uniform( EPS, 100.0 );

s[ i ] = uniform( EPS, 100.0 );

x[ i ] = uniform( 0.0, 1.0 );

a[ i ] = uniform( EPS, 5.0 );

s[ i ] = uniform( EPS, 5.0 );

anandkaranubc · 2025-02-03T03:02:05Z

+	len = 100;
+	x = new Float64Array( len );
+	for ( i = 0; i < len; i++ ) {
+		x[ i ] = uniform( EPS, 2.0 );


Suggested change

x[ i ] = uniform( EPS, 2.0 );

x[ i ] = uniform( -2.0, 2.0 );

anandkaranubc · 2025-02-03T03:02:50Z

+		x[ i ] = uniform( EPS, 2.0 );
+		a[ i ] = uniform( EPS, 100.0 );
+		s[ i ] = uniform( EPS, 100.0 );


Same change as above. This also applies to other functions, where applicable ( for eg. logpdf, etc.)

anandkaranubc · 2025-02-03T03:05:13Z

+var uniform = require( '@stdlib/random/base/uniform' );
+var Float64Array = require( '@stdlib/array/float64' );
 var isnan = require( '@stdlib/math/base/assert/is-nan' );
+var EPS = require( '@stdlib/constants/float64/eps' );


Is EPS needed here?

anandkaranubc · 2025-02-03T03:06:37Z

+	var alpha;
+	var beta;
+	var len;


Suggested change

var alpha;

var beta;

var len;

var len;

var a;

var s;

anandkaranubc · 2025-02-03T03:07:18Z

+	alpha = new Float64Array( len );
+	beta = new Float64Array( len );
+	for ( i = 0; i < len; i++ ) {
+		x[ i ] = uniform( EPS, 2.0 );
+		alpha[ i ] = uniform( EPS, 100.0 );
+		beta[ i ] = uniform( EPS, 100.0 );


Suggested change

alpha = new Float64Array( len );

beta = new Float64Array( len );

for ( i = 0; i < len; i++ ) {

x[ i ] = uniform( EPS, 2.0 );

alpha[ i ] = uniform( EPS, 100.0 );

beta[ i ] = uniform( EPS, 100.0 );

a = new Float64Array( len );

s = new Float64Array( len );

for ( i = 0; i < len; i++ ) {

x[ i ] = uniform( EPS, 2.0 );

a[ i ] = uniform( EPS, 100.0 );

s[ i ] = uniform( EPS, 100.0 );

anandkaranubc · 2025-02-03T03:07:53Z

-		a = ( randu()*100.0 ) + EPS;
-		s = ( randu()*100.0 ) + EPS;
-		y = pdf( x, a, s );
+		y = pdf( x[ i % len ], alpha[ i % len ], beta[ i % len ] );


Suggested change

y = pdf( x[ i % len ], alpha[ i % len ], beta[ i % len ] );

y = pdf( x[ i % len ], a[ i % len ], s[ i % len ] );

anandkaranubc · 2025-02-03T03:08:09Z

+	var alpha;
+	var beta;
+	var len;


Same changes as above.

anandkaranubc · 2025-02-03T18:36:26Z

Like I do the think there was something wrong in the native implementation for the kumaraswamy.median package as majority of the test cases are failing due to extremely low tolerance.

Great catch! This is the related PR: stdlib-js/stdlib#4020.

I just checked:

This line:

stdlib/lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/median/src/main.c

Line 42 in 367ec2c

return 1.0 - stdlib_base_pow( 0.5, 1.0 / b );

should be:

return stdlib_base_pow( 1.0 - stdlib_base_pow( 2.0, -1.0 / b ), 1.0 / a );

The original line is only correct when a = 1.0, but it should be modified to account for any positive value of a.

Just checked it locally too, this fix also makes all native tests pass. A PR addressing this would be a great addition!

Neerajpathak07 · 2025-02-03T18:42:41Z

Like I do the think there was something wrong in the native implementation for the kumaraswamy.median package as majority of the test cases are failing due to extremely low tolerance.

Great catch! This is the related PR: stdlib-js/stdlib#4020.

I just checked:

This line:

stdlib/lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/median/src/main.c

Line 42 in 367ec2c

return 1.0 - stdlib_base_pow( 0.5, 1.0 / b );

should be:
return stdlib_base_pow( 1.0 - stdlib_base_pow( 2.0, -1.0 / b ), 1.0 / a );
The original line is only correct when a = 1.0, but it should be modified to account for any positive value of a.

Just checked it locally too, this fix also makes all native tests pass. A PR addressing this would be a great addition!

Yeah, and additionally we do need to add a few test cases to implicitly test a few technicalities what do you say?

anandkaranubc · 2025-02-03T18:50:16Z

Yeah, and additionally we do need to add a few test cases to implicitly test a few technicalities what do you say?

Do you have any in mind?

I think test.native.js (both test files, actually) are quite exhaustive in checking when the function returns NaN and also in validating against 1000 Julia fixtures.

Neerajpathak07 · 2025-02-03T19:33:44Z

Yeah, and additionally we do need to add a few test cases to implicitly test a few technicalities what do you say?

Do you have any in mind?

I think test.native.js (both test files, actually) are quite exhaustive in checking when the function returns NaN and also in validating against 1000 Julia fixtures.

Yeah, that's correct I was thinking of testing if the function returns NaN on passing different values as parameters in it.
To have a broader idea on how it explicitly handles it.

yuvimittal · 2025-02-04T13:21:42Z

Like I do the think there was something wrong in the native implementation for the kumaraswamy.median package as majority of the test cases are failing due to extremely low tolerance.

Great catch! This is the related PR: stdlib-js/stdlib#4020.

I just checked:

This line:

stdlib/lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/median/src/main.c

Line 42 in 367ec2c

return 1.0 - stdlib_base_pow( 0.5, 1.0 / b );

should be:
return stdlib_base_pow( 1.0 - stdlib_base_pow( 2.0, -1.0 / b ), 1.0 / a );
The original line is only correct when a = 1.0, but it should be modified to account for any positive value of a.

Just checked it locally too, this fix also makes all native tests pass. A PR addressing this would be a great addition!

Even after changing the test cases aren't still passing in my system . I will have to go through the test cases once . Are you sure it is passing in locally in your system? @anandkaranubc

anandkaranubc · 2025-02-04T18:20:25Z

Even after changing the test cases aren't still passing in my system

Once you change anything in the C implementation side, you will have to build the native addon again

NODE_ADDONS_PATTERN="@stdlib/stats/base/dists/kumaraswamy/median" make install-node-addons

Planeshifter · 2025-06-22T23:39:17Z

We have moved to using var uniform = require( '@stdlib/random/array/uniform' ); instead of var uniform = require( '@stdlib/random/base/uniform' ); to directly draw arrays filled with numbers from the desired distribution. Since this PR follows our older convention and now has merge conflicts, I am going to close it. Thanks for your contribution either way, it is appreciated!

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 2, 2025

yuvimittal changed the title ~~feat(benchmarks): refactor random number generation in JS benchmarks for stats/base/dists/kumaraswamy~~ feat : refactor random number generation in JS benchmarks for stats/base/dists/kumaraswamy Feb 2, 2025

headlessNode changed the title ~~feat : refactor random number generation in JS benchmarks for stats/base/dists/kumaraswamy~~ bench: refactor random number generation in stats/base/dists/kumaraswamy Feb 2, 2025

yuvimittal changed the title ~~bench: refactor random number generation in stats/base/dists/kumaraswamy~~ feat : refactor random number generation in stats/base/dists/kumaraswamy Feb 2, 2025

Neerajpathak07 reviewed Feb 3, 2025

View reviewed changes

Comment thread lib/node_modules/@stdlib/stats/base/dists/kumaraswamy/mode/benchmark/benchmark.js

kgryte changed the title ~~feat : refactor random number generation in stats/base/dists/kumaraswamy~~ bench: refactor random number generation in stats/base/dists/kumaraswamy Feb 3, 2025

anandkaranubc requested changes Feb 3, 2025

View reviewed changes

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

anandkaranubc mentioned this pull request Feb 3, 2025

[BUG]: C implementation for stats/base/dists/kumaraswamy/median has different results than the JavaScript implementation #5047

Closed

2 tasks

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

stdlib-bot removed the First-time Contributor A pull request from a contributor who has never previously committed to the project repository. label Feb 15, 2025

yuvimittal closed this Apr 1, 2025

stdlib-bot added the Good First PR A pull request resolving a Good First Issue. label Apr 1, 2025

yuvimittal reopened this Apr 1, 2025

Planeshifter closed this Jun 22, 2025

	y = pdf( x[ i % len ], alpha[ i % len ], beta[ i % len ] );
	y = pdf( x[ i % len ], a[ i % len ], s[ i % len ] );

Uh oh!

Conversation

yuvimittal commented Feb 2, 2025

Description

Related Issues

Questions

Other

Checklist

Uh oh!

Neerajpathak07 commented Feb 2, 2025

Uh oh!

yuvimittal commented Feb 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Neerajpathak07 commented Feb 3, 2025

Uh oh!

Neerajpathak07 commented Feb 3, 2025

Uh oh!

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

Uh oh!

Uh oh!

yuvimittal commented Feb 3, 2025

Uh oh!

Neerajpathak07 commented Feb 3, 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!

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!

anandkaranubc commented Feb 3, 2025

Uh oh!

Neerajpathak07 commented Feb 3, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

yuvimittal commented Feb 4, 2025

Uh oh!

anandkaranubc commented Feb 4, 2025

Uh oh!

Planeshifter commented Jun 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Neerajpathak07 commented Feb 2, 2025 •

edited

Loading

yuvimittal commented Feb 3, 2025 •

edited

Loading

Neerajpathak07 commented Feb 3, 2025 •

edited

Loading

yuvimittal commented Feb 3, 2025 •

edited

Loading

Neerajpathak07 commented Feb 3, 2025 •

edited

Loading

anandkaranubc commented Feb 3, 2025 •

edited

Loading

Neerajpathak07 commented Feb 3, 2025 •

edited

Loading