-
-
Notifications
You must be signed in to change notification settings - Fork 860
bench: update random value generation #7748
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
base: develop
Are you sure you want to change the base?
bench: update random value generation #7748
Conversation
…ts/triangular stdlib-js#4989 Resolves stdlib-js#4989
Coverage Report
The above coverage report was generated for the changes in this PR. |
Hi @anandkaranubc, I’ve made sure all benchmarks pass locally and I’ve addressed all the lint errors that showed up so far (spacing, import order, header formatting, etc.). |
@AryanJ18 Thank you for your PR. However, your proposed changes contain significant deviations from our style conventions. I suppose going back through and comparing to other packages to ensure that everything looks and feels like other packages (e.g., in terms of spacing, etc.). |
var as = new Float64Array(bm.iterations); | ||
var bs = new Float64Array(bm.iterations); |
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 changed to follow what is done in https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/stats/base/dists/arcsine/ctor/benchmark/benchmark.js
Like:
var dist;
var len;
var a;
var b;
var c;
var i;
len = 100;
a = new Float64Array( len );
b = new Float64Array( len );
c = new Float64Array( len );
for ( i = 0; i < len; i++ ) {
a[ i ] = uniform( EPS, 10.0 );
b[ i ] = uniform( a[ i ] + EPS, a[ i ] + 10.0 + EPS );
c[ i ] = uniform( a[ i ], b[ i ] );
}
c = ( randu() * ( b-a ) ) + a; | ||
a = as[i]; | ||
b = bs[i]; | ||
c = cs[i]; | ||
dist = new Triangular( a, b, c ); |
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.
dist = new Triangular( a, b, c ); | |
dist = new Triangular( a[ i%len ], b[ i%len ], c[ i%len ] ); |
After the above changes have been made.
@@ -98,7 +110,7 @@ bench( pkg+'::set:a', function benchmark( bm ) { | |||
|
|||
bm.tic(); | |||
for ( i = 0; i < bm.iterations; i++ ) { | |||
y = ( 100.0*randu() ) + EPS; | |||
y = uniform(EPS, 100.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 needs to be moved outside the benchmarking loop, similar to what's being done above.
lib/node_modules/@stdlib/stats/base/dists/triangular/ctor/benchmark/benchmark.js
Show resolved
Hide resolved
max[ i ] = ( randu()*10.0 ) + min[ i ] + EPS; | ||
mode[ i ] = ( ( max[ i ] - min[ i ] ) * randu() ) + min[ i ]; | ||
min[ i ] = uniform( 0.0, 10.0 ); | ||
max[ i ] = uniform( min[ i ] + EPS, 10.0 + min[ i ] + EPS ); |
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.
max[ i ] = uniform( min[ i ] + EPS, 10.0 + min[ i ] + EPS ); | |
max[ i ] = uniform( min[ i ] + EPS, min[ i ] + 10.0 + EPS ); |
Just to be consistent with the other benchmarks. This change applies throughout the PR.
max[ i ] = ( randu()*10.0 ) + min[ i ] + EPS; | ||
mode[ i ] = ( ( max[ i ] - min[ i ] ) * randu() ) + min[ i ]; | ||
min[ i ] = uniform( 0.0, 10.0 ); | ||
max[ i ] = uniform( min[ i ] + EPS, 10.0 + min[ i ] + EPS ); |
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 as above. Rest looks good :)
@AryanJ18 The PR looks in good shape overall. Thank you for making the changes as Athan suggested. Just a few changes remain, as mentioned above. |
Thank you! I’ll make the remaining changes as suggested and push the update shortly. |
Resolves #4989
Description
This pull request:
Related Issues
Questions
No.
Other
In the issue it is mentioned to use uniform and discreteUniform to replace randu but I did not find any case in which discreteUniform could be used as per my knowledge
Checklist
@stdlib-js/reviewers