Skip to content
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

test: Adapt benchmarks to use codspeed #741

Merged
merged 16 commits into from
Aug 12, 2024
Merged

Conversation

DeaMariaLeon
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • πŸͺ„ Feature
  • 🐞 Bug Fix
  • πŸ”§ Optimization
  • πŸ“š Documentation
  • πŸ§ͺ Test
  • πŸ› οΈ Other

Related issues

  • Related issue #
  • Closes #

Checklist

  • Code follows style guide
  • Tests added
  • Documented the changes

Please explain your changes below.

Adapted benchmarks from original file benchmark_coo.py that was used to benchmark with asv. Now benchmarking with codspeed.

Copy link

codspeed-hq bot commented Aug 9, 2024

CodSpeed Performance Report

Merging #741 will not alter performance

Comparing DeaMariaLeon:bench2 (5c34b67) with main (0612373)

Summary

βœ… 14 untouched benchmarks

πŸ†• 14 new benchmarks

Benchmarks breakdown

Benchmark main DeaMariaLeon:bench2 Change
πŸ†• test_elemwise_broadcast[side=100-add] N/A 14 ms N/A
πŸ†• test_elemwise_broadcast[side=100-mul] N/A 3.1 ms N/A
πŸ†• test_elemwise_broadcast[side=1000-add] N/A 14.9 s N/A
πŸ†• test_elemwise_broadcast[side=1000-mul] N/A 73.5 ms N/A
πŸ†• test_elemwise_broadcast[side=500-add] N/A 1.7 s N/A
πŸ†• test_elemwise_broadcast[side=500-mul] N/A 12.5 ms N/A
πŸ†• test_index_fancy[side=100] N/A 3.5 ms N/A
πŸ†• test_index_scalar[side=100] N/A 481.2 Β΅s N/A
πŸ†• test_index_slice2[side=100] N/A 1.6 ms N/A
πŸ†• test_index_slice3[side=100] N/A 1.4 ms N/A
πŸ†• test_index_slice[side=100] N/A 1.1 ms N/A
πŸ†• test_matmul[side=1000] N/A 39.9 ms N/A
πŸ†• test_matmul[side=100] N/A 2.7 ms N/A
πŸ†• test_matmul[side=500] N/A 8 ms N/A

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

A couple of comments. Nice work overall!

benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

Small changes -- otherwise LGTM.

benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
benchmarks/test_benchmark_coo.py Outdated Show resolved Hide resolved
@DeaMariaLeon
Copy link
Collaborator Author

@hameerabbasi Regarding:

SEED = 42
I'd import this from a central place, so it can be changed just once.

It's not working the way I did it on this commit. But locally I did something else: I put them inside a conftest.py. Which it seems to work, except that I noticed that SEED and DENSITY vary on other files. Like elemwise_example.py, matmul_example.py, etc.

@hameerabbasi
Copy link
Collaborator

For seed, let's unify it. For density, we can leave it constant in each file where it's constant; and change it where it should be changed.

@DeaMariaLeon
Copy link
Collaborator Author

For seed, let's unify it.

What does this mean? set just one value for all the tests, on all the files? Set the value on conftest.py?

@hameerabbasi
Copy link
Collaborator

Set the value on conftest.py?

Set one value for everything in some central place.

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

One final change and we should be good to go. The changes discussed out-of-band can take place on this PR or a follow-up, as you wish.

benchmarks/utils.py Outdated Show resolved Hide resolved
@DeaMariaLeon
Copy link
Collaborator Author

I'll do the out-of-band changes on another PR if that's fine with you then.

@hameerabbasi hameerabbasi enabled auto-merge (squash) August 12, 2024 09:31
@hameerabbasi hameerabbasi merged commit 29f9632 into pydata:main Aug 12, 2024
18 of 19 checks passed
@DeaMariaLeon DeaMariaLeon deleted the bench2 branch August 12, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants