-
Notifications
You must be signed in to change notification settings - Fork 122
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
Add performance benchmarks #748
Add performance benchmarks #748
Conversation
@TomAugspurger Would be great to get your feedback as well. |
Codecov ReportBase: 94.43% // Head: 94.43% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
=======================================
Coverage 94.43% 94.43%
=======================================
Files 83 83
Lines 12056 12056
Branches 1143 1143
=======================================
Hits 11385 11385
Misses 492 492
Partials 179 179 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
LGTM. Did you have a running list of other scenarios you'd like to benchmark? One or two that spring to mind are:
- Mutating the entire tree (e.g. setting the root href and updating all of the links/self hrefs)
- Summary/extent computation
Thanks @gadomski, I have a bit of a list started in #729. I like both of these suggestions, I'll add them to the PR. |
I'm of the opinion that we _shouldn't_ run benchmarks on Github runners, so I'm removing this workflow.
This lets simple command like `asv dev` work out of the box.
I'm not really sure how useful this is, but it was asked for so at least we have something.
This doesn't run benchmarks, but just checks to make sure they build.
I picked this PR up and, in the interest of not letting perfect be the enemy of good, implemented most of @duckontheweb's suggestions:
I didn't implement:
cc @duckontheweb would love your review if you have the bandwidth, I can't request b/c it's your PR. IMO this PR should be squash-merged since it's one coherent set of changes w/ some fixup commits -- I can't rebase b/c it's on @duckontheweb's fork. |
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.
Looking good. This is a nice addition.
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.
Looks good.
Related Issue(s):
Description:
Uses the airspeed velocity (
asv
) library to run performance benchmarks. This configuration generally follows the strategy used byxarray
(as documented here), with some notable exceptions:Bench
base class and can be changed for a particular benchmark.--interleave-rounds
option (also seems to help with eliminating false positives)This is a work-in-progress, but I want to get feedback on the approach before fleshing out the rest.
Still to do:
PR Checklist:
pre-commit run --all-files
)scripts/test
)