-
-
Notifications
You must be signed in to change notification settings - Fork 835
bench: refactor random number generation in stats/base/dists/signrank
#5088
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
Conversation
--- 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 ---
Hello! Thank you for your contribution to stdlib. We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:
This acknowledgment confirms that you've read the guidelines, which include:
We can't review or accept contributions without this acknowledgment. Thank you for your understanding and cooperation. We look forward to reviewing your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Hi there! 👋
And thank you for opening your first pull request! We will review it shortly. 🏃 💨
Coverage Report
The above coverage report was generated for the changes in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR, @shraddha761! To complete the linked issue, please also make the following changes:
Move random number generation out of the benchmarking loops and initialize it before the benchmarks.
See the linked PR for how this would look like.
--- 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 did some changes as required |
var nValues; | ||
var values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var nValues; | |
var values; | |
var len; | |
var n; |
Please add only len
and keep the original variable names unchanged (for consistency).
var i; | ||
var y; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var i; | |
var y; | |
var y; | |
var i; |
var uniform = require( '@stdlib/random/base/uniform' ); | ||
var discreteUniform = require( '@stdlib/random/base/discrete-uniform' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var uniform = require( '@stdlib/random/base/uniform' ); | |
var discreteUniform = require( '@stdlib/random/base/discrete-uniform' ); | |
var Float64Array = require( '@stdlib/array/float64' ); | |
var uniform = require( '@stdlib/random/base/uniform' ); | |
var discreteUniform = require( '@stdlib/random/base/discrete-uniform' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Float64Array
should be used instead of Array
values = new Array( b.iterations ); | ||
nValues = new Array( b.iterations ); | ||
for ( i = 0; i < b.iterations; i++ ) { | ||
values[ i ] = uniform( 0.0, 20.0 ); | ||
nValues[ i ] = discreteUniform( 1, 20 ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values = new Array( b.iterations ); | |
nValues = new Array( b.iterations ); | |
for ( i = 0; i < b.iterations; i++ ) { | |
values[ i ] = uniform( 0.0, 20.0 ); | |
nValues[ i ] = discreteUniform( 1, 20 ); | |
} | |
len = 100; | |
x = new Float64Array( len ); | |
n = new Float64Array( len ); | |
for ( i = 0; i < len; i++ ) { | |
x[ i ] = uniform( 0.0, 20.0 ); | |
n[ i ] = discreteUniform( 1, 20 ); | |
} |
x = values[ i ]; | ||
y = cdf( x, nValues[ i ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x = values[ i ]; | |
y = cdf( x, nValues[ i ] ); | |
y = cdf( x[ i % len ], n[ i % len ] ); |
@@ -54,6 +61,7 @@ bench( pkg, function benchmark( b ) { | |||
}); | |||
|
|||
bench( pkg+':factory', function benchmark( b ) { | |||
var values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here and also applies throughout the PR.
values = new Array( b.iterations ); | ||
for ( i = 0; i < b.iterations; i++ ) { | ||
values[ i ] = uniform( 0.0, 20.0 ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be updated too by following the example above.
var pkg = require( './../package.json' ).name; | ||
var quantile = require( './../lib' ); | ||
var EPS = 1e-10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EPS
is not needed in this benchmark file, as the original code uses:
p = randu();
Thus, using p = uniform( 0.0, 1.0 );
is perfectly fine in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is ever used, it should be imported as:
var EPS = require( '@stdlib/constants/float64/eps' );
Thanks, @shraddha761, for working on this issue. I've left some feedback and an example for you to review. I hope that helps. You may also find the reference issues provided in #4986 useful too. Can you also change the title to: bench: refactor random number generation in Note: There are backticks around |
stats/base/dists/signrank
This pull request has been automatically closed because it has been inactive for an extended period after changes were requested. If you still wish to pursue this contribution, feel free to reopen the pull request or submit a new one. We appreciate your interest in contributing to stdlib! |
Resolves #4986
Description
This pull request:
Related Issues
This pull request:
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers