Skip to content

bench: update benchmarks to measure affirmative/negative test values #1428

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 11, 2024
Merged

bench: update benchmarks to measure affirmative/negative test values #1428

merged 5 commits into from
Sep 11, 2024

Conversation

soumajit23
Copy link
Contributor

@soumajit23 soumajit23 commented Mar 1, 2024 •
edited by kgryte
Loading

Partially resolves #1148

Description

What is the purpose of this pull request?

This pull request:

  • Refactors assert/is-cube-number benchmarks to measure affirmative/negative test values in separate benchmark runs to have a better handle on performance depending on the input value type.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Benchmark results after refactor: isCubeNumberBenchmarks.txt

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

stdlib-bot
stdlib-bot reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@stdlib-bot stdlib-bot left a 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. 🏃 💨

@Pranavchiku
Copy link
Member

Hi @soumajit23 can you please populate the PR description so that we can identify what this PR is trying to do?

@soumajit23
Copy link
Contributor Author

Hi @soumajit23 can you please populate the PR description so that we can identify what this PR is trying to do?

Hi @Pranavchiku! thanks for the feedback. Will do.

@soumajit23 soumajit23 changed the title refactor(is-cube-number): refactor the is-cube-number benchmarks to… refactor(is-cube-number): refactor the is-cube-number benchmarks to measure affirmative/negative test values. Mar 1, 2024
@soumajit23
Copy link
Contributor Author

@Pranavchiku please check the description.
Thanks in advance.

Pranavchiku
Pranavchiku suggested changes Mar 2, 2024
View reviewed changes
Copy link
Member

@Pranavchiku Pranavchiku left a comment

Choose a reason for hiding this comment

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

This looks good, apply changes and this PR will be ready to merge.

@kgryte kgryte changed the title refactor(is-cube-number): refactor the is-cube-number benchmarks to measure affirmative/negative test values. bench: update benchmarks to measure affirmative/negative test values Mar 2, 2024
@kgryte kgryte added Benchmarks Pull requests adding or improving benchmarks for measuring performance. Utilities Issue or pull request concerning general utilities. labels Mar 2, 2024
kgryte
kgryte requested changes Mar 2, 2024
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@soumajit23 Thank you for working on this. A couple of comments:

  1. When adding benchmarks measuring perf for true and false cases, the values need to actually evaluate as those values. E.g., for the true case, valid values are 8, 27, 64, etc.
  2. You removed isObject and isPrimitive benchmarks. These should be restored. The expectation is that this PR should double the number of benchmarks by adding additional benchmarks for each API (the main export, isPrimitive, and isObject).

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Mar 2, 2024
@soumajit23
Copy link
Contributor Author

soumajit23 commented Mar 2, 2024 •
edited
Loading

image
@kgryte @Pranavchiku I am getting this error message while trying to push. Please tell me how to fix this error.
image
this is the part that is showing the error I believe.

@kgryte
Copy link
Member

kgryte commented Mar 2, 2024

@soumajit23 Don't rebase unless you need to. In this case, you don't need to pull in the latest upstream changes for what you are working on.

@kgryte
Copy link
Member

kgryte commented Mar 2, 2024

I don't think the errors are due to your code. This seems to have been caused by a recent merge which contained a bug in its example code (cc @Planeshifter and @Pranavchiku).

@soumajit23
Copy link
Contributor Author

@kgryte please check.

@soumajit23
Copy link
Contributor Author

isCubeNumberBenchmarks(NEW).txt
these are the new benchmark results

@soumajit23
Copy link
Contributor Author

@Pranavchiku @kgryte hello,
it has been a while, could you please review the changes and let me know if any more changes are required?

@soumajit23 soumajit23 mentioned this pull request Mar 28, 2024
Closed
6 tasks
@soumajit23
bench: update benchmark.js
ab86e06 
Signed-off-by: Soumajit Chatterjee <[email protected]>
@Planeshifter Planeshifter self-requested a review September 7, 2024 19:26
Planeshifter
Planeshifter approved these changes Sep 7, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Updated the benchmark values. PR should be ready to land.

@Planeshifter Planeshifter added Ready To Merge A pull request which is ready to be merged. and removed Needs Changes Pull request which needs changes before being merged. labels Sep 7, 2024
@Planeshifter Planeshifter merged commit 1f6fc8b into stdlib-js:develop Sep 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Planeshifter Planeshifter Planeshifter approved these changes

@Pranavchiku Pranavchiku Awaiting requested review from Pranavchiku

@kgryte kgryte Awaiting requested review from kgryte

@stdlib-bot stdlib-bot Awaiting requested review from stdlib-bot

Assignees
No one assigned
Labels
Benchmarks Pull requests adding or improving benchmarks for measuring performance. Ready To Merge A pull request which is ready to be merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: Update @stdlib/assert/* benchmarks to measure affirmative/negative test values
5 participants
@soumajit23 @Pranavchiku @kgryte @Planeshifter @stdlib-bot