Skip to content

added median#19

Open
PirosB3 wants to merge 3 commits into
django:masterfrom
PirosB3:feature/median
Open

added median#19
PirosB3 wants to merge 3 commits into
django:masterfrom
PirosB3:feature/median

Conversation

@PirosB3
Copy link
Copy Markdown

@PirosB3 PirosB3 commented Jul 4, 2014

No description provided.

Copy link
Copy Markdown
Contributor

@acdha acdha left a comment

Choose a reason for hiding this comment

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

A couple of minor changes

Comment thread djangobench/main.py
experiment_data=experiment_data,
)
print(format_benchmark_result(result, len(control_data.runtimes)))
print(format_benchmark_result(result, len(control_data.runtimes), control_data.runtimes, experiment_data.runtimes, show_median))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line is getting pretty long - perhaps it'd be best to break it on the new trailing comma so it'd be easier to scan – maybe like this?

print(format_benchmark_result(result, len(control_data.runtimes),
                              control_data.runtimes, …

Comment thread djangobench/main.py
sorted_runs = sorted(runs)
middle = len(runs) / 2
if len(runs) % 2 == 0:
return sum(sorted_runs[middle-1:middle+1])/2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I generally find this a little easier to read with spaces around the operators

Comment thread djangobench/main.py


def format_benchmark_result(result, num_points):
def format_benchmark_result(result, num_points, control_data, experiment_data, show_median):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether show_median should be a keyword argument here as well - that list is getting a bit long but it's not too bad.

@acdha
Copy link
Copy Markdown
Contributor

acdha commented Nov 9, 2016

I left a couple of minor stylistic points above. A larger question: the basic part is whether we should use statistics.median on Python 3 (there appears to be a backport version on PyPI for Python 2), but I was also wondering whether we should be including something other than the median as well – either the variance or something like 50th & 95th percentile values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants