lincolnloop / python-package-manager-shootout

Benchmarking various Python package managers
https://lincolnloop.github.io/python-package-manager-shootout/
MIT License
44 stars 10 forks source link

Stop setting Y-axis `suggestedMax` option #18

Closed edmorley closed 8 months ago

edmorley commented 8 months ago

Previously all of the graphs used the same custom Y-axis maximum value, regardless of the results being shown on that specific install/lock/update/add-package graph.

This custom Y-axis value is calculated by taking the maximum dataset value across all graphs and datasets, rounding it up to the next integer and then adding 10 seconds to it. In addition, it appears that when using the suggestedMax option chartjs adds its own padding to the max value, rounding it up even further to the next major "axis tick" marker.

For example, with today's results, the maximum datapoint across all graphs and datasets is 62.19 seconds duration, which gives a suggestedMax of 73, which results in chartjs rounding up to a Y-axis max of 80 seconds (since the major tick markers for a number of that size, are every 10 seconds).

This results in lots of whitespace on some of the graphs, and can hide how big of a performance gap exists between some of the tools. This came up previously in #11 and #12.

To resolve this, we need to change two things:

  1. Stop adding the 10 seconds padding, given that chartjs already adds its own padding
  2. Switch to using a graph-local maximum (ie: each graph has its own maximum Y-axis value that's calculated independently of the other graphs)

The initial version of this PR implemented this by adjusting csv_to_json.py to add new graph-specific graphs[key]["max"] values, and had site/index.js use those for suggestedMax (and that also removed the 10 seconds padding).

However, when I compared that approach to just omitting the suggestedMax value entirely, the results were identical.

ie: It seems that chartjs's default values are already what we want. (I had presumed that suggestedMax was originally needed because of the custom min/max/stddev markers, but chartjs appears to take those UI elements into account when calculating the Y-axis range already.)

As such, this PR now simply removes the custom suggestedMax values entirely (and the related processing in csv_to_json.py) to give the same result.

Relevant chartjs docs (though sadly they don't document things like the auto-padding and default behaviour): https://www.chartjs.org/docs/latest/axes/#common-options-to-all-axes https://www.chartjs.org/docs/latest/axes/#axis-range-settings

Refs #11. Fixes #12.

edmorley commented 8 months ago

Before

install-before add-before

After

install-after add-after
ipmb commented 8 months ago

Thanks @edmorley!