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.
LebedevRI
left a comment
There was a problem hiding this comment.
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. |
|
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.