-
-
Notifications
You must be signed in to change notification settings - Fork 804
bench: refactor random number generation in stats/base/dists/laplace
#5270
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
…ase/dists/laplace
Coverage Report
The above coverage report was generated for the changes in this PR. |
@anandkaranubc @kgryte please review this PRs |
@anandkaranubc @Planeshifter ready for review |
@anandkaranubc please review this PRs |
On it! |
thanks |
lib/node_modules/@stdlib/stats/base/dists/laplace/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/cdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/pdf/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
@gkbishnoi07, Can you also make the same changes that were made in #5296? Specifically, the The title should be updated to (for consistency): bench: refactor random number generation in |
Sure, I'll update you soon. |
stats/base/dists/laplace
stats/base/dists/laplace
@anandkaranubc please review this PRs |
@gkbishnoi07 Can you point out the files that need this change? Note that this change is only needed where the random number generation occurs outside the benchmarking loops. Why? Because in cases like these, specifically in the |
@anandkaranubc Got it! I just noticed that some files still have random generation inside the benchmarking loop instead of using a uniform distribution. First, I'm working on updating those files, and along the way, I'll implement insan(dist.*). |
Yes, and this change is being tracked in #4993. Ideally, this change should already be reflected in the respective PRs. If you have other PRs related to this tracking issue, please update them as well. Thanks! |
Ok sure! |
@anandkaranubc, are you reviewing this PR now? |
@anandkaranubc i applied all changes please look out this PR |
@anandkaranubc please review this PRs |
On it! |
lib/node_modules/@stdlib/stats/base/dists/laplace/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/dists/laplace/median/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
@gkbishnoi07 The PR still needs some changes. |
@anandkaranubc changes applied please review this PRs |
lib/node_modules/@stdlib/stats/base/dists/laplace/ctor/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
It's also good practice to keep your commit messages consistent with code conventions. Here is the guide |
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.
Please also confirm if the same change is consistent throughout the PR.
lib/node_modules/@stdlib/stats/base/dists/laplace/quantile/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.
Everything looks good now! Thanks for working on this.
@anandkaranubc stdlib bot again give Needs Review label? |
Yes, the PR needs to go through the maintainers before it can be merged. |
Okay,thanks |
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 for your PR and @anandkaranubc for review. Let's get this one in. 🚀
PR Commit Message
Please review the above commit message and make any necessary adjustments. |
Resolves #4976
Description
What is the purpose of this pull request?
This pull request:
stats/base/dists/laplace
.randu()
withuniform()
for cleaner and more consistent code.Related Issues
Does this pull request have any related issues?
This pull request:
stats/base/dists/laplace
#4976Questions
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.