Skip to content

[RFC]: Refactor random number generation in JS benchmarks for stats/base/dists/poisson #4984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
3 tasks done
anandkaranubc opened this issue Jan 31, 2025 · 6 comments · Fixed by #5106
Closed
3 tasks done
Labels
Benchmarks Pull requests adding or improving benchmarks for measuring performance. difficulty: 1 Low degree of difficulty. Should be straightforward to implement and/or resolve. Good First Issue A good first issue for new contributors! JavaScript Issue involves or relates to JavaScript. RFC Request for comments. Feature requests and proposed changes.

Comments

@anandkaranubc
Copy link
Contributor

Description

This RFC proposes improving random number generation in JS benchmarks for stats/base/dists/poisson.

Context: At present, in the remaining packages, random number generation in JS benchmarks occurs inside the benchmarking loop. Since random number generation is an expensive operation, it should be moved out of the benchmarking loops and initialized beforehand to avoid interfering with the results.

When adding support, the following tasks should be completed:

  1. Move random number generation out of the benchmarking loops and initialize it before the benchmarks.

  2. Ensure that the generated random values use the same range as the existing values to maintain consistency.

  3. Use uniform and discreteUniform from @stdlib/random/base/uniform and @stdlib/random/base/discrete-uniform instead of randu expressions.

    • For example, replace ( randu() * 10.0 + EPS ) with uniform( EPS, 10.0 ).
    • Replace ceil( randu() * 10.0 + EPS ) with discreteUniform( 1, 10 ).

To provide a concrete example of what a PR implementing the desired changes should contain, see #4837 and #4955, which provide examples for using both uniform and discreteUniform.

Related Issues

#4837, #4955

Questions

No.

Other

Once the implementation is complete, you should be able to run the following make commands:

Build Native Add-on

NODE_ADDONS_PATTERN="@stdlib/stats/base/dists/poisson/*" make install-node-addons

Run JavaScript Benchmarks

make benchmark-javascript-files FILES="$(pwd)/lib/node_modules/@stdlib/stats/base/dists/poisson/*/benchmark/benchmark.js"

Run JavaScript Native Benchmarks

make benchmark-javascript-files FILES="$(pwd)/lib/node_modules/@stdlib/stats/base/dists/poisson/*/benchmark/benchmark.native.js"

Note: If running benchmarks results in an error, it is likely due to the random number generators producing values where the functions are not defined. To resolve this, check the relevant benchmark files, adjust the input value ranges to ensure they remain within the domain where the functions are valid, and then rerun the benchmarks. Reference: #4955

Checklist

  • I have read and understood the Code of Conduct.
  • Searched for existing issues and pull requests.
  • The issue name begins with RFC:.
@anandkaranubc anandkaranubc added RFC Request for comments. Feature requests and proposed changes. difficulty: 1 Low degree of difficulty. Should be straightforward to implement and/or resolve. Benchmarks Pull requests adding or improving benchmarks for measuring performance. Good First Issue A good first issue for new contributors! JavaScript Issue involves or relates to JavaScript. labels Jan 31, 2025
@gagan-bhullar-tech
Copy link

I would like to work on this issue. Please assign.

@Krishna-Sharma-g
Copy link
Contributor

Krishna-Sharma-g commented Feb 6, 2025

hello @anandkaranubc i would like to work on this so basically i have to change every benchmark like as you said for every file inside the poisson like cdf , ctor , entropy and all so i have done it for one that is cdf's benchmark can you please check that i have to do it like this or i have to do something else

Image

like here i changed the randu with the uniform and the ciel one with the discreteUniform one and also i initialized them before the benchmarking loop

Image

can you please share your thoughts !!

@Krishna-Sharma-g
Copy link
Contributor

@kgryte can you also review it please

@anandkaranubc
Copy link
Contributor Author

can you please share your thoughts !!

Thanks, @gautam-krishna-sharma, for looking into how to solve this issue. Here are the changes you need to make:

  • The variable names will remain the same. xValues and lambdaValues should still be called x and lambda.
  • Use Float64Array instead of Array for array initialization.
  • The lengths of the arrays should be set using a len variable, initialized to 100, instead of b.iterations.
  • You do not need to add comments explaining what is being done.
  • Ensure that you follow the code conventions, such as adding extra space between the start and end of brackets, like x[ i ] = discreteUniform( 1, 100 ).

The reference issues will be a good starting point to guide your changes. Good luck!

@Krishna-Sharma-g
Copy link
Contributor

Krishna-Sharma-g commented Feb 6, 2025

okay thankyou @anandkaranubc i will surely do these changes and will look into the reference issue

@pranav-1720
Copy link
Contributor

Created a PR following the examples of the other refactors.

saurabhraghuvanshii pushed a commit to saurabhraghuvanshii/stdlib-gs that referenced this issue Feb 11, 2025
ShabiShett07 pushed a commit to ShabiShett07/stdlib that referenced this issue Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmarks Pull requests adding or improving benchmarks for measuring performance. difficulty: 1 Low degree of difficulty. Should be straightforward to implement and/or resolve. Good First Issue A good first issue for new contributors! JavaScript Issue involves or relates to JavaScript. RFC Request for comments. Feature requests and proposed changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants