Aggregates: use non-aggregate count as iteration count. #706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It is incorrect to say that an aggregate is computed over run's iterations, because those iterations already got averaged. Similarly, if there are N repetitions with 1 iterations each, an aggregate will be computed over N measurements, not 1. Thus it is best to simply use the count of separate reports. Fixes google#586.
|
✅ Build benchmark 1530 completed (commit 1e959e4d75 by @LebedevRI) |
|
Can you provide a screenshot/link to the resulting output before and after so it's capture in the PR? |
dmah42
approved these changes
Oct 18, 2018
|
The tests convey that i think.. |
|
Hm, that is clearly broken. |
This is a rather huge mess. What is worse, the custom counters are already divided by iteration count by now. And i'm not sure it is in general correct to compute the stats over sum(iterations), and then divide the stat by iteration count. I think we should cleanup everything to divide by iteration count as early as possible.
|
Ok, that was a fun issue :) Test added. |
dmah42
approved these changes
Oct 18, 2018
|
Yay, thank you for the review! |
EricWF
pushed a commit
to efcs/benchmark
that referenced
this pull request
Nov 29, 2018
It is incorrect to say that an aggregate is computed over run's iterations, because those iterations already got averaged. Similarly, if there are N repetitions with 1 iterations each, an aggregate will be computed over N measurements, not 1. Thus it is best to simply use the count of separate reports. Fixes google#586.
JBakamovic
pushed a commit
to JBakamovic/benchmark
that referenced
this pull request
Dec 6, 2018
It is incorrect to say that an aggregate is computed over run's iterations, because those iterations already got averaged. Similarly, if there are N repetitions with 1 iterations each, an aggregate will be computed over N measurements, not 1. Thus it is best to simply use the count of separate reports. Fixes google#586.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is incorrect to say that an aggregate is computed over
run's iterations, because those iterations already got averaged.
Similarly, if there are N repetitions with 1 iterations each,
an aggregate will be computed over N measurements, not 1.
Thus it is best to simply use the count of separate reports.
Fixes #586.