Conversation
|
Hmmm nice, looks like your did some refactorings, I'll try find time this week to review. |
Very tiny ones, separated into the first commit for ease of review. ef81697 |
src/pytest_benchmark/cli.py
Outdated
| add_display_options(compare_command.add_argument, prefix='') | ||
| add_histogram_options(compare_command.add_argument, prefix='') | ||
| compare_command.add_argument( | ||
| '--compare-between', |
There was a problem hiding this comment.
The prefix seems to be forced to '' for all compare subcommand options (and there's no prefix available in this function as far as I can see..?)?
There was a problem hiding this comment.
Ah oops I kinda forgot what this function was for. What I wanted is --compare-between to be --between instead (cause the command already has "compare" - pointless to just repeat "compare" all over).
|
Sorry for the delays, I finally got to try this and I have an example to discuss. I have this stuff locally (bunch of crappy stats for 2 platforms): If I run So the way I understand it I think it would be better is the differences appear right after the column that the diff is made of. Currently it's: Also, about columns, I would like to propose this idea: make --between be exclusive with --columns. Cause it doesn't make sense to use --between with all the columns unless you're comparing just 2 runs. Actually I am not sure but the defaults are producing too many columns. It should default somehow to either compare 1 stat or compare all the default stats on just 2 runs. Maybe go with these:
|
|
So lemme know what you think, to sum it up, my ideas where to compact/remove redundant info from the column names (like Chg:runname just repeats runname from previous column) and give better defaults/prevent users from outputting something with 100 columns by default. |
Exactly. Maybe the 1st item's header should have a
Sure, good idea. I admit I just basically use
I use OPS, but I'm not sure it's the best metric to default to. 🤔 |
|
Reworked the column order, made the reference numbers blue, added the reference indicator. 
I didn't touch the argument parsing yet though, not sure about the (implicit) default. |
That delta sign is better but can you reorder the columns? Then you don't need the |
Maybe no defaults then, and make |
So | mean@REF | mean@A | Δmean@A | mean@B | Δmean@B | min@REF | min@A | Δmin@A | min@B | Δmin@B | Okay! That'll work OK with the blue color for the reference.
? Sounds good to me 👍 |
|
|
|
Looks great. Two last things to do I think:
|
|
Switched to using name_format and added a screenshot (meticulously squeezed to 32 colors and optimized 😁). |
2 participants
This PR adds a
compare --betweenmode, effectively a pivot table between 2..N result files.I needed this for django/asgiref#551 and cleaned it up for general use :)
Example output with color: