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

CI: Add Benchmarking #46

Merged
merged 9 commits into from
Aug 6, 2024
Merged

CI: Add Benchmarking #46

merged 9 commits into from
Aug 6, 2024

Conversation

Jacob-Stevens-Haas
Copy link
Collaborator

Hey Andy, I've added some logging to an experiment and found that some derivative methods can be slow. This isn't a surprise, but the enterprise solution is to add benchmarking to CI so that we can start quantifying improvements. This uses Airspeed Velocity (asv), the same benchmarking package as pandas.

Like most PRs that deal with CI, this one may have a variety of commits to try to get it working. I don't know yet how to feed asv previous benchmark results so that it can know performance improvements/regressions.

@Jacob-Stevens-Haas Jacob-Stevens-Haas force-pushed the benchmarking branch 4 times, most recently from 1e81d67 to 6c2389c Compare August 1, 2024 01:47
@Jacob-Stevens-Haas Jacob-Stevens-Haas force-pushed the benchmarking branch 5 times, most recently from a6fe9e4 to ab16d37 Compare August 1, 2024 02:33
@Jacob-Stevens-Haas
Copy link
Collaborator Author

Jacob-Stevens-Haas commented Aug 1, 2024

Ok, it looks like the basic benchmarking is working. You can see that the commit with an intentional performance regression failed CI.

Two things I'd like to add:

  • test memory useage for finite difference
  • asv publish artifact so that it's possible to browse the results of the benchmarking action (It'll look like pandas, but with only the merge-base and PR as the two commits) This'll have to wait for a later PR if it's desired.

I'd like to just leave this with a test for finite difference as a template later expansion and improvement. I think the PR is ready to go now

@Jacob-Stevens-Haas Jacob-Stevens-Haas marked this pull request as ready for review August 1, 2024 03:25
Copy link
Owner

@andgoldschmidt andgoldschmidt left a comment

Choose a reason for hiding this comment

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

This is great, and will hopefully encourage contributions.

@andgoldschmidt andgoldschmidt merged commit 4440536 into master Aug 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants