-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make json and csv output consistent. #1662
Conversation
Currently, the --benchmark_format=csv option does not output the correct value for the cv statistics. Also, the json output should not contain a time unit for the cv statistics.
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.
Also, the json output should not contain a time unit for the cv statistics.
That statement is lacking motivational proof.
Please drop the JSON change, it is intentional.
The CSV reporter is deprecated, not sure how much we care about it,
but the change seems innoculus...
The cv is the "coefficient of variation" (https://en.wikipedia.org/wiki/Coefficient_of_variation), which is a dimensionless number (https://en.wikipedia.org/wiki/Dimensionless_quantity). So the time unit should either be removed, or it should be empty. Currently, it contains "ns", which is definitely wrong. |
Yep, i know that, i added it.
I'm sorry, this is not productive. I'm telling you it is there intentionally, and you ignored that comment. |
I'm sorry. Would you mind sharing why the time unit for the cv is intentionally "ns" even though it is a dimensionless quantity? Also note that the console output does not contain a time unit for the cv. Is the console output intentionally inconsistent with the json output? |
|
Ok, apparently i forgot. There is |
It works just fine, though, if I set the time_unit to the empty string instead of removing it. I have reverted the JSON change for now, though I'm not yet really convinced yet that there isn't a better option.
This is very surprising (and begs the question what it does mean then). In particular, given that there is a field with the identical name It would be really helpful if such non-obvious and non-intuitive design choices could be documented somewhere. |
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.
The csv change seems fine to me, but we really are ought to make a decision
on the future of that reporter. Is it deprecated with no new functionality
to be accepted or not? @dmah42
the plan of record is: don't add new functionality because we should instead rely on the JSON output and have a separate tool to create the CSV from JSON. this got stuck behind an aborted effort to have a v2 that had some deeper API changes. i don't have a strong opinion on whether we should continue to try to deprecate the CSV output or not, tbh. while it's here and in use, we should at least make sure it's correct though. |
Currently, the --benchmark_format=csv option does not output the correct value for the cv statistics. Also, the json output should not contain a time unit for the cv statistics.