-
-
Notifications
You must be signed in to change notification settings - Fork 613
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
bench: refactor random number generation in JS benchmarks for stats/base/dists/logistic
#5139
Conversation
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. |
7acbcad
to
36f5573
Compare
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
36f5573
to
f56669b
Compare
@anandkaranubc ready for review |
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
I noticed that you adjusted the spacing in the files, including areas of the code that do not require changes to resolve this issue. That is unnecessary. Reference PRs will help clarify which changes are required. |
Thanks for the feedback! I’ll revert the unnecessary spacing changes and ensure only the required modifications are included. Appreciate the guidance! |
@anandkaranubc ready for review |
@anandkaranubc please review this |
@Planeshifter please review |
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/entropy/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
@gkbishnoi07, I have left some comments for you to review. Good luck! |
Thank you! I'll review your comments and get back to you soon. |
@anandkaranubc Can you please review #5193 |
@anandkaranubc ready for review |
@anandkaranubc any updates |
@Planeshifter please review this PR |
@kgryte can you please review this PRs |
On it! |
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 PR still requires several changes. I have pinpointed the exact areas in my review. You can request another review from me after making those changes. Just ensure that you are 100% certain all the necessary modifications have been made. Good luck!
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/kurtosis/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/stdev/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/stdev/benchmark/benchmark.native.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/variance/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/variance/benchmark/benchmark.native.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/variance/benchmark/benchmark.native.js
Outdated
Show resolved
Hide resolved
ok,thanks for review |
Hi @anandkaranubc, could you please review these PRs when you have time? I'm eager to contribute more to this repo. Thank you! |
@Planeshifter @kgryte please review this PRs |
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.
Some more changes are needed. The rest looks good!
lib/node_modules/@stdlib/stats/base/dists/logistic/logcdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/logistic/logpdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
@anandkaranubc please review |
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.
The PR looks ready to be merged. Thanks!
PR Commit Message
Please review the above commit message and make any necessary adjustments. |
@gkbishnoi07 Can you also update the title to: bench: refactor random number generation in JS benchmarks for
After this, you can wait for a maintainer to merge this PR. |
Yes, sure! Thanks for reviewing my PRs. |
stats/base/dists/logistic
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 @gkbishnoi07 and @anandkaranubc!
Description
What is the purpose of this pull request?
This pull request:
stats/base/dists/logistic
.randu()
withuniform()
for cleaner and more consistent code.Related Issues
Does this pull request have any related issues?
This pull request:
stats/base/dists/logistic
#4978Questions
Any questions for reviewers of this pull request?
No.
Other
Any other information relevant to this pull request?
No.
Checklist
Please ensure the following tasks are completed before submitting this pull request.