Skip to content

bench: refactor random number generation in stats/base/dists/weibull #5011

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
wants to merge 2 commits into from

Conversation

ShabiShett07
Copy link
Contributor

Resolves #4992

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

What about the benchmark of ctor? Do we need to change that too?

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.


@stdlib-js/reviewers

@stdlib-bot
Copy link
Contributor

stdlib-bot commented Feb 1, 2025

Coverage Report

Package Statements Branches Functions Lines
stats/base/dists/weibull/cdf $\color{green}222/222$
$\color{green}+100.00\%$
$\color{green}23/23$
$\color{green}+100.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{green}222/222$
$\color{green}+100.00\%$
stats/base/dists/weibull/entropy $\color{green}126/126$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}126/126$
$\color{green}+100.00\%$
stats/base/dists/weibull/kurtosis $\color{green}137/137$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}137/137$
$\color{green}+100.00\%$
stats/base/dists/weibull/logcdf $\color{green}235/235$
$\color{green}+100.00\%$
$\color{green}26/26$
$\color{green}+100.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{green}235/235$
$\color{green}+100.00\%$
stats/base/dists/weibull/logpdf $\color{green}236/236$
$\color{green}+100.00\%$
$\color{green}33/33$
$\color{green}+100.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{green}236/236$
$\color{green}+100.00\%$
stats/base/dists/weibull/mean $\color{green}125/125$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}125/125$
$\color{green}+100.00\%$
stats/base/dists/weibull/median $\color{green}126/126$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}126/126$
$\color{green}+100.00\%$
stats/base/dists/weibull/mgf $\color{red}262/264$
$\color{green}+99.24\%$
$\color{red}23/25$
$\color{green}+92.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{red}262/264$
$\color{green}+99.24\%$
stats/base/dists/weibull/mode $\color{green}128/128$
$\color{green}+100.00\%$
$\color{green}10/10$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}128/128$
$\color{green}+100.00\%$
stats/base/dists/weibull/pdf $\color{green}238/238$
$\color{green}+100.00\%$
$\color{green}34/34$
$\color{green}+100.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{green}238/238$
$\color{green}+100.00\%$
stats/base/dists/weibull/quantile $\color{green}218/218$
$\color{green}+100.00\%$
$\color{green}23/23$
$\color{green}+100.00\%$
$\color{green}3/3$
$\color{green}+100.00\%$
$\color{green}218/218$
$\color{green}+100.00\%$
stats/base/dists/weibull/skewness $\color{green}139/139$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}139/139$
$\color{green}+100.00\%$
stats/base/dists/weibull/stdev $\color{green}128/128$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}128/128$
$\color{green}+100.00\%$
stats/base/dists/weibull/variance $\color{green}128/128$
$\color{green}+100.00\%$
$\color{green}8/8$
$\color{green}+100.00\%$
$\color{green}1/1$
$\color{green}+100.00\%$
$\color{green}128/128$
$\color{green}+100.00\%$

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

@anandkaranubc
Copy link
Contributor

Can you please change the title to:

bench: refactor random number generation in stats/base/dists/weibull

@anandkaranubc
Copy link
Contributor

Also, I am not sure if you have the most recent changes in your current branch. Try to update your branch with the latest changes so you can also apply the required changes to all the benchmark.native.js files. Good luck! :)

@anandkaranubc anandkaranubc added the Needs Changes Pull request which needs changes before being merged. label Feb 3, 2025
@stdlib-bot stdlib-bot added the Statistics Issue or pull request related to statistical functionality. label Feb 3, 2025
@ShabiShett07 ShabiShett07 changed the title feat: refactor random number generation stats/base/dists/weibull bench: refactor random number generation in stats/base/dists/weibull Feb 6, 2025
@ShabiShett07
Copy link
Contributor Author

Hey @anandkaranubc, Thank you for reviewing my PR.
I tried to solve the errors in my PR, and I think I solved all the errors you mentioned. Please have a look at it!

Comment on lines +79 to +81
len = 100;

x = new Float64Array( len );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
len = 100;
x = new Float64Array( len );
len = 100;
x = new Float64Array( len );

No space between initializations

Comment on lines 51 to 53
b.tic();

for ( i = 0; i < b.iterations; i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
b.tic();
for ( i = 0; i < b.iterations; i++ ) {
b.tic();
for ( i = 0; i < b.iterations; i++ ) {

Comment on lines 84 to 86
b.tic();

for ( i = 0; i < b.iterations; i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
b.tic();
for ( i = 0; i < b.iterations; i++ ) {
b.tic();
for ( i = 0; i < b.iterations; i++ ) {

Comment on lines 51 to 53
b.tic();

for ( i = 0; i < b.iterations; i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
b.tic();
for ( i = 0; i < b.iterations; i++ ) {
b.tic();
for ( i = 0; i < b.iterations; i++ ) {

This change is required throughout the PR.

lambda = new Float64Array( len );
k = new Float64Array( len );
for ( i = 0; i < len; i++ ) {
t[ i ] = uniform( EPS, 2.0 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t[ i ] = uniform( EPS, 2.0 );
t[ i ] = uniform( EPS, 1.0 );

Make sure the minimum and maximum values in the uniform expressions are no different than the original ones.

for ( i = 0; i < len; i++ ) {
t[ i ] = uniform( EPS, 2.0 );
lambda[ i ] = uniform( EPS, 2.0 );
k[ i ] = uniform(1.0, 2.0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
k[ i ] = uniform(1.0, 2.0);
k[ i ] = uniform( 1.0, 2.0 );

@anandkaranubc
Copy link
Contributor

Also, I am not sure if you have the most recent changes in your current branch. Try to update your branch with the latest changes so you can also apply the required changes to all the benchmark.native.js files. Good luck! :)

This still needs to be addressed.

@kgryte kgryte added the Duplicate This issue or pull request already exists. label Apr 2, 2025
@kgryte
Copy link
Member

kgryte commented Apr 2, 2025

Ref: #5342

@kgryte kgryte added the autoclose: Already Resolved Pull request which should be auto-closed due proposed changes duplicating already included changes. label Apr 2, 2025
@stdlib-bot
Copy link
Contributor

Thank you for working on this pull request. However, we cannot accept your contribution as the issue this pull request seeks to resolve has already been addressed in a different pull request or commit.

Thank you again for your interest in stdlib, and we look forward to reviewing your future contributions.

@stdlib-bot stdlib-bot closed this Apr 2, 2025
@stdlib-bot stdlib-bot added the Good First PR A pull request resolving a Good First Issue. label Apr 2, 2025
@ShabiShett07 ShabiShett07 deleted the feature/weibull branch April 3, 2025 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoclose: Already Resolved Pull request which should be auto-closed due proposed changes duplicating already included changes. Duplicate This issue or pull request already exists. Good First PR A pull request resolving a Good First Issue. Needs Changes Pull request which needs changes before being merged. Statistics Issue or pull request related to statistical functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: Refactor random number generation in JS benchmarks for stats/base/dists/weibull
4 participants