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

Add percentage diff column to compare subcommand #1333

Merged
merged 7 commits into from
Sep 22, 2021
Merged

Add percentage diff column to compare subcommand #1333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa8c8e7
First pass
b-deam Sep 15, 2021
3bacec7
Second pass
b-deam Sep 16, 2021
b34dbc5
Fix tests
b-deam Sep 16, 2021
178dffd
Revert changes
b-deam Sep 16, 2021
ecf53fb
I should add linting on save
b-deam Sep 16, 2021
b3ec514
Implement suggested changes
b-deam Sep 22, 2021
d38a00f
I should add lint and format on save
b-deam Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions esrally/reporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ def write_single_report(report_file, report_format, cwd, numbers_align, headers,
formatter = format_as_csv
else:
raise exceptions.SystemSetupError("Unknown report format '%s'" % report_format)

print_internal(formatter(headers, data_rich))
if len(report_file) > 0:
normalized_report_file = rio.normalize_path(report_file, cwd)
Expand Down Expand Up @@ -379,7 +378,7 @@ def _write_report(self, metrics_table, metrics_table_console):
self.report_format,
self.cwd,
self.numbers_align,
headers=["Metric", "Task", "Baseline", "Contender", "Diff", "Unit"],
headers=["Metric", "Task", "Baseline", "Contender", "Diff", "Unit", "Diff %"],
data_plain=metrics_table,
data_rich=metrics_table_console,
)
Expand Down Expand Up @@ -864,15 +863,18 @@ def _line(self, metric, baseline, contender, task, unit, treat_increase_as_impro
formatter(contender),
self._diff(baseline, contender, treat_increase_as_improvement, formatter),
unit,
self._diff(baseline, contender, treat_increase_as_improvement, formatter, as_percentage=True),
]
else:
return []

def _diff(self, baseline, contender, treat_increase_as_improvement, formatter=lambda x: x):
def _diff(self, baseline, contender, treat_increase_as_improvement, formatter=lambda x: x, as_percentage=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, on my first review, I hesitated to mention that adding a boolean parameter to a function is usually considered to be a code smell. But now that Daniel suggested a refactoring, I'm less shy. :) https://adamj.eu/tech/2021/07/10/python-type-hints-how-to-avoid-the-boolean-trap/ explains why that's a problem and what the alternatives are in modern Python.

(It's fine in this case as it's a small private function only called twice: I'm not asking to change your code. But maybe you'll find those references useful.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the references! They're great. It's funny because my first (uncommitted) approach was actually to implement this as a seperate private function called diff_percentage, but I was worried that there was too much code duplication.

def identity(x):
return x

diff = formatter(contender - baseline)
def _safe_divide(n, d):
return n / d if d else 0

if self.plain:
color_greater = identity
color_smaller = identity
Expand All @@ -886,10 +888,22 @@ def identity(x):
color_smaller = console.format.green
color_neutral = console.format.neutral

if diff > 0:
return color_greater("+%.5f" % diff)
elif diff < 0:
return color_smaller("%.5f" % diff)
if as_percentage:
b-deam marked this conversation as resolved.
Show resolved Hide resolved
diff = formatter(_safe_divide(contender - baseline, baseline) * 100.0)
precision = 2
suffix = "%"
else:
diff = formatter(contender - baseline)
precision = 5
suffix = ""

# ensures that numbers that appear as "zero" are also colored neutrally
threshold = 10 ** -precision
formatted = f"{diff:.{precision}f}{suffix}"

if diff >= threshold:
return color_greater(f"+{formatted}")
elif diff <= -threshold:
return color_smaller(formatted)
else:
# tabulate needs this to align all values correctly
return color_neutral("%.5f" % diff)
return color_neutral(formatted)
8 changes: 4 additions & 4 deletions tests/reporter_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ def setUp(self):
self.empty_header = ["Header"]
self.empty_data = []

self.metrics_header = ["Metric", "Task", "Baseline", "Contender", "Diff", "Unit"]
self.metrics_header = ["Metric", "Task", "Baseline", "Contender", "Diff", "Unit", "Diff %"]
self.metrics_data = [
["Min Throughput", "index", "17300", "18000", "700", "ops/s"],
["Median Throughput", "index", "17500", "18500", "1000", "ops/s"],
["Max Throughput", "index", "17700", "19000", "1300", "ops/s"],
["Min Throughput", "index", "17300", "18000", "700", "ops/s", "4.04%"],
["Median Throughput", "index", "17500", "18500", "1000", "ops/s", "5.71%"],
["Max Throughput", "index", "17700", "19000", "1300", "ops/s", "7.34%"],
]
self.numbers_align = "right"

Expand Down