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.

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

CI Benchmarking reports opposite results #395

Closed
sbryngelson opened this issue Apr 10, 2024 · 5 comments · Fixed by #414
Closed

CI Benchmarking reports opposite results #395

sbryngelson opened this issue Apr 10, 2024 · 5 comments · Fixed by #414
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers

Comments

@sbryngelson
Copy link
Member

The CI Benchmarker reports master is X times faster than PR (or something like this) but in reality it is actually reporting master is X times slower than PR. This is a lhs vs rhs swap issue in bench.py.
@sbryngelson
Copy link
Member Author

we were interpreting the results incorrectly, and the current message is technically correct but confusing. a more clear wording of this message would be helpful (or just an additional sentence like 1.5x indicates the PR is twice as fast as Master, 0.5x the PR is twice as slow. @anandrdbz

@anandrdbz
Copy link
Contributor

Hmm I guess the problem is either way you're gonna run into an issue when x < 1, since it's much more natural to interpret values > 1. And the problem is this line doesn't actually use a value for x, it is simply written to indicate what the table values are.

@sbryngelson
Copy link
Member Author

We could add a comment/line before the table that reads

For example: 1.5x indicates the PR is 1.5-times faster than Master; 0.5x indicates the PR is 0.5-times as fast as Master (so, slower)

@anandrdbz
Copy link
Contributor

Okay I can do that, and maybe we should do it from the perspective of the PR anyways since master is the baseline (so PR is x times slower/faster than master instead of master is x times slower/faster than PR)

@sbryngelson
Copy link
Member Author

That's fine as long as it's correct and is instructive

@sbryngelson sbryngelson added documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers labels May 1, 2024
@okBrian okBrian mentioned this issue May 14, 2024
Merged
19 tasks
sbryngelson pushed a commit that referenced this issue May 25, 2024
@okBrian
Fix for issue #395 (#414)
d292c29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for issue #395 okBrian/MFC
2 participants
@sbryngelson @anandrdbz