-
Notifications
You must be signed in to change notification settings - Fork 97
Improve flatten benchmark #5360
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5360 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 5
Lines ? 115
Branches ? 0
========================================
Hits ? 115
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
47f38fa to
0d2f984
Compare
0d2f984 to
8067701
Compare
| N = int(np.round(np.sqrt(N)) ** 2) | ||
| trimmed_N = int(np.round(np.sqrt(N)) ** 2) | ||
| sqrt_N = int(np.sqrt(N)) | ||
| shape = (sqrt_N, sqrt_N) |
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.
Comment only, since what I'm about to point out is old code, not your change.
The multiple calls to np.sqrt, and the unneeded call to np.round, bother me. I would rewrite this as:
sqrt_N = int(np.sqrt(N))
trimmed_N = sqrt_N * sqrt_N
shape = (sqrt_N, sqrt_N)
But it certainly works as written, and one extra call to np.sqrt isn't going to noticeably affect the benchmark run time, so ... comment only.
Since this isn't meant to be something that requires resolution, feel free to resolve it yourself as you see fit.
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.
Nice fix. I left a comment, but it's purely a comment on the code that predates this change. It wasn't meant to require resolution.
Improve flatten benchmark by:
trimmed_N, so thatNwill not differ frompytest.N.flattenbenchmark from thebenchmark.ini, as it has been moved to theoptionalfolder.shape_typeto thebenchmark.extra_info["description"].