fix: show units on curve, line and scatter axis labels#963
Merged
Conversation
Follow-up to #960: histogram gained units there and band/bar/heatmap/ surface/distribution already showed them. This closes the remaining gaps flagged in the audit — curve_result.py, line_result.py and scatter_result.py set no axis labels at all. Adds a shared label_with_units() helper in bencher.utils that follows the existing 'ul' = unitless convention from sweep_base, and applies it: - _build_curve_overlay (covers to_curve and the aggregated line path) - to_line_ds (float-x path, 0D over_time path) and _to_line_tap_ds - _to_scatter_ds Labels are applied with kwargs.setdefault so explicit user xlabel/ylabel kwargs still win.
Contributor
Reviewer's GuideAdds a shared label_with_units() helper and uses it to apply unit-aware axis labels to curve, line, and scatter plots, while preserving user-specified labels and adding tests to verify behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Performance Report for
|
| Metric | Value |
|---|---|
| Total tests | 1665 |
| Total time | 127.33s |
| Mean | 0.0765s |
| Median | 0.0020s |
Top 10 slowest tests
| Test | Time (s) |
|---|---|
test.test_bench_examples.TestBenchExamples::test_example_meta |
18.373 |
test.test_over_time_save_perf::test_save_faster_without_aggregated_tab |
5.590 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_drift.py] |
4.649 |
test.test_generated_examples::test_generated_example[cartesian_animation/example_cartesian_animation.py] |
3.255 |
test.test_split_render_examples::test_split_render_subprocess_media |
3.196 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_step.py] |
2.915 |
test.test_hash_persistent.TestCrossProcessDeterminism::test_hash_stable_across_two_processes[ResultBool] |
2.861 |
test.test_generated_examples::test_generated_example[result_types/result_image/example_result_image_to_video.py] |
2.856 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_noise.py] |
2.834 |
test.test_over_time_repeats.TestMaxSliderPoints::test_default_subsampling_caps_at_max |
2.454 |
Updated by Performance Tracking workflow
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
LineResult.to_line_dsand_to_line_tap_ds, accessingself.plt_cnt_cfg.float_vars[0]assumes at least one float var is present; consider guarding this or mirroring the safer lookup-by-name pattern used in_build_curve_overlayto avoid potentialIndexErrorin edge configurations. - The new
label_with_unitshelper assumes the presence of a.nameattribute and a string-like.units; if this might be reused more broadly, consider adding a fallback (e.g.,getattr(var, "name", str(var))) to make it more robust to non-parameter inputs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LineResult.to_line_ds` and `_to_line_tap_ds`, accessing `self.plt_cnt_cfg.float_vars[0]` assumes at least one float var is present; consider guarding this or mirroring the safer lookup-by-name pattern used in `_build_curve_overlay` to avoid potential `IndexError` in edge configurations.
- The new `label_with_units` helper assumes the presence of a `.name` attribute and a string-like `.units`; if this might be reused more broadly, consider adding a fallback (e.g., `getattr(var, "name", str(var))`) to make it more robust to non-parameter inputs.
## Individual Comments
### Comment 1
<location path="test/test_axis_units.py" line_range="96-106" />
<code_context>
+ self.assertEqual(label_with_units(_LabelBench.param.unitless), "unitless")
+
+
+class TestCurveAxisUnits(unittest.TestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.res = _run_sweep(FloatBench, "units_curve", ["distance"], ["throughput"], repeats=2)
+
+ def test_curve_axis_labels_show_units(self):
+ curve = _find_element(self.res.to_curve(), hv.Curve)
+ self.assertIsNotNone(curve)
+ opts = curve.opts.get().kwargs
+ self.assertEqual(opts["xlabel"], "distance [m]")
+ self.assertEqual(opts["ylabel"], "throughput [ops/s]")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to verify that explicit xlabel/ylabel kwargs override the auto-generated labels.
Since the tests only cover auto-populated labels, please also add a case calling `self.res.to_curve(xlabel="custom x", ylabel="custom y")` and asserting those values are preserved. This will lock in the precedence behavior of explicit labels over `label_with_units` and guard against future regressions.
```suggestion
class TestCurveAxisUnits(unittest.TestCase):
@classmethod
def setUpClass(cls):
cls.res = _run_sweep(FloatBench, "units_curve", ["distance"], ["throughput"], repeats=2)
def test_curve_axis_labels_show_units(self):
curve = _find_element(self.res.to_curve(), hv.Curve)
self.assertIsNotNone(curve)
opts = curve.opts.get().kwargs
self.assertEqual(opts["xlabel"], "distance [m]")
self.assertEqual(opts["ylabel"], "throughput [ops/s]")
def test_explicit_axis_labels_override_units(self):
curve = _find_element(
self.res.to_curve(xlabel="custom x", ylabel="custom y"),
hv.Curve,
)
self.assertIsNotNone(curve)
opts = curve.opts.get().kwargs
self.assertEqual(opts["xlabel"], "custom x")
self.assertEqual(opts["ylabel"], "custom y")
```
</issue_to_address>
### Comment 2
<location path="test/test_axis_units.py" line_range="109-119" />
<code_context>
+ self.assertEqual(opts["ylabel"], "throughput [ops/s]")
+
+
+class TestLineAxisUnits(unittest.TestCase):
+ @classmethod
+ def setUpClass(cls):
+ cls.res = _run_sweep(FloatBench, "units_line", ["distance"], ["throughput"], repeats=1)
+
+ def test_line_axis_labels_show_units(self):
+ curve = _find_element(self.res.to_line(use_tap=False), hv.Curve)
+ self.assertIsNotNone(curve)
+ opts = curve.opts.get().kwargs
+ self.assertEqual(opts["xlabel"], "distance [m]")
+ self.assertEqual(opts["ylabel"], "throughput [ops/s]")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the tap-based line path (use_tap=True) to exercise `_to_line_tap_ds`.
The current test only covers `to_line(use_tap=False)` (float-x path), but this PR also adds labeling in `_to_line_tap_ds`. Please add another test or parametrize this one to also call `self.res.to_line(use_tap=True)` and assert the same xlabel/ylabel values, so the tap-based path is exercised and its axis units remain covered.
```suggestion
class TestLineAxisUnits(unittest.TestCase):
@classmethod
def setUpClass(cls):
cls.res = _run_sweep(FloatBench, "units_line", ["distance"], ["throughput"], repeats=1)
def _assert_line_axis_labels(self, curve):
self.assertIsNotNone(curve)
opts = curve.opts.get().kwargs
self.assertEqual(opts["xlabel"], "distance [m]")
self.assertEqual(opts["ylabel"], "throughput [ops/s]")
def test_line_axis_labels_show_units_float(self):
curve = _find_element(self.res.to_line(use_tap=False), hv.Curve)
self._assert_line_axis_labels(curve)
def test_line_axis_labels_show_units_tap(self):
curve = _find_element(self.res.to_line(use_tap=True), hv.Curve)
self._assert_line_axis_labels(curve)
```
</issue_to_address>
### Comment 3
<location path="test/test_axis_units.py" line_range="84-93" />
<code_context>
+ unitless = bn.ResultFloat(units="ul")
+
+
+class TestLabelWithUnits(unittest.TestCase):
+ def test_name_and_units(self):
+ self.assertEqual(label_with_units(_LabelBench.param.with_units), "with_units [ms]")
+
+ def test_no_units(self):
+ self.assertEqual(label_with_units(_LabelBench.param.no_units), "no_units")
+
+ def test_unitless_convention(self):
+ """'ul' is the sweep-variable convention for unitless and must not be shown."""
+ self.assertEqual(label_with_units(_LabelBench.param.unitless), "unitless")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a label_with_units test for objects without a `units` attribute and for `units=None`.
To fully cover the `getattr(var, "units", "")` behavior, please add tests for (1) an object with only a `name` attribute and no `units`, and (2) a result var with `units=None`. Both should yield just the name, confirming correct handling of non-param objects and `None` units.
Suggested implementation:
```python
class _LabelBench(bn.ParametrizedSweep):
"""Params must be class-bound for param to assign their names."""
with_units = bn.ResultFloat(units="ms")
no_units = bn.ResultFloat(units="")
unitless = bn.ResultFloat(units="ul")
none_units = bn.ResultFloat(units=None)
```
```python
class TestLabelWithUnits(unittest.TestCase):
def test_name_and_units(self):
self.assertEqual(label_with_units(_LabelBench.param.with_units), "with_units [ms]")
def test_no_units(self):
self.assertEqual(label_with_units(_LabelBench.param.no_units), "no_units")
def test_unitless_convention(self):
"""'ul' is the sweep-variable convention for unitless and must not be shown."""
self.assertEqual(label_with_units(_LabelBench.param.unitless), "unitless")
def test_object_without_units_attribute(self):
class Dummy:
name = "without_units"
self.assertEqual(label_with_units(Dummy()), "without_units")
def test_none_units(self):
"""Result vars with units=None should also yield just the name."""
self.assertEqual(label_with_units(_LabelBench.param.none_units), "none_units")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+96
to
+106
| class TestCurveAxisUnits(unittest.TestCase): | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| cls.res = _run_sweep(FloatBench, "units_curve", ["distance"], ["throughput"], repeats=2) | ||
|
|
||
| def test_curve_axis_labels_show_units(self): | ||
| curve = _find_element(self.res.to_curve(), hv.Curve) | ||
| self.assertIsNotNone(curve) | ||
| opts = curve.opts.get().kwargs | ||
| self.assertEqual(opts["xlabel"], "distance [m]") | ||
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test to verify that explicit xlabel/ylabel kwargs override the auto-generated labels.
Since the tests only cover auto-populated labels, please also add a case calling self.res.to_curve(xlabel="custom x", ylabel="custom y") and asserting those values are preserved. This will lock in the precedence behavior of explicit labels over label_with_units and guard against future regressions.
Suggested change
| class TestCurveAxisUnits(unittest.TestCase): | |
| @classmethod | |
| def setUpClass(cls): | |
| cls.res = _run_sweep(FloatBench, "units_curve", ["distance"], ["throughput"], repeats=2) | |
| def test_curve_axis_labels_show_units(self): | |
| curve = _find_element(self.res.to_curve(), hv.Curve) | |
| self.assertIsNotNone(curve) | |
| opts = curve.opts.get().kwargs | |
| self.assertEqual(opts["xlabel"], "distance [m]") | |
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") | |
| class TestCurveAxisUnits(unittest.TestCase): | |
| @classmethod | |
| def setUpClass(cls): | |
| cls.res = _run_sweep(FloatBench, "units_curve", ["distance"], ["throughput"], repeats=2) | |
| def test_curve_axis_labels_show_units(self): | |
| curve = _find_element(self.res.to_curve(), hv.Curve) | |
| self.assertIsNotNone(curve) | |
| opts = curve.opts.get().kwargs | |
| self.assertEqual(opts["xlabel"], "distance [m]") | |
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") | |
| def test_explicit_axis_labels_override_units(self): | |
| curve = _find_element( | |
| self.res.to_curve(xlabel="custom x", ylabel="custom y"), | |
| hv.Curve, | |
| ) | |
| self.assertIsNotNone(curve) | |
| opts = curve.opts.get().kwargs | |
| self.assertEqual(opts["xlabel"], "custom x") | |
| self.assertEqual(opts["ylabel"], "custom y") |
Comment on lines
+109
to
+119
| class TestLineAxisUnits(unittest.TestCase): | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| cls.res = _run_sweep(FloatBench, "units_line", ["distance"], ["throughput"], repeats=1) | ||
|
|
||
| def test_line_axis_labels_show_units(self): | ||
| curve = _find_element(self.res.to_line(use_tap=False), hv.Curve) | ||
| self.assertIsNotNone(curve) | ||
| opts = curve.opts.get().kwargs | ||
| self.assertEqual(opts["xlabel"], "distance [m]") | ||
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for the tap-based line path (use_tap=True) to exercise _to_line_tap_ds.
The current test only covers to_line(use_tap=False) (float-x path), but this PR also adds labeling in _to_line_tap_ds. Please add another test or parametrize this one to also call self.res.to_line(use_tap=True) and assert the same xlabel/ylabel values, so the tap-based path is exercised and its axis units remain covered.
Suggested change
| class TestLineAxisUnits(unittest.TestCase): | |
| @classmethod | |
| def setUpClass(cls): | |
| cls.res = _run_sweep(FloatBench, "units_line", ["distance"], ["throughput"], repeats=1) | |
| def test_line_axis_labels_show_units(self): | |
| curve = _find_element(self.res.to_line(use_tap=False), hv.Curve) | |
| self.assertIsNotNone(curve) | |
| opts = curve.opts.get().kwargs | |
| self.assertEqual(opts["xlabel"], "distance [m]") | |
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") | |
| class TestLineAxisUnits(unittest.TestCase): | |
| @classmethod | |
| def setUpClass(cls): | |
| cls.res = _run_sweep(FloatBench, "units_line", ["distance"], ["throughput"], repeats=1) | |
| def _assert_line_axis_labels(self, curve): | |
| self.assertIsNotNone(curve) | |
| opts = curve.opts.get().kwargs | |
| self.assertEqual(opts["xlabel"], "distance [m]") | |
| self.assertEqual(opts["ylabel"], "throughput [ops/s]") | |
| def test_line_axis_labels_show_units_float(self): | |
| curve = _find_element(self.res.to_line(use_tap=False), hv.Curve) | |
| self._assert_line_axis_labels(curve) | |
| def test_line_axis_labels_show_units_tap(self): | |
| curve = _find_element(self.res.to_line(use_tap=True), hv.Curve) | |
| self._assert_line_axis_labels(curve) |
Comment on lines
+84
to
+93
| class TestLabelWithUnits(unittest.TestCase): | ||
| def test_name_and_units(self): | ||
| self.assertEqual(label_with_units(_LabelBench.param.with_units), "with_units [ms]") | ||
|
|
||
| def test_no_units(self): | ||
| self.assertEqual(label_with_units(_LabelBench.param.no_units), "no_units") | ||
|
|
||
| def test_unitless_convention(self): | ||
| """'ul' is the sweep-variable convention for unitless and must not be shown.""" | ||
| self.assertEqual(label_with_units(_LabelBench.param.unitless), "unitless") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a label_with_units test for objects without a units attribute and for units=None.
To fully cover the getattr(var, "units", "") behavior, please add tests for (1) an object with only a name attribute and no units, and (2) a result var with units=None. Both should yield just the name, confirming correct handling of non-param objects and None units.
Suggested implementation:
class _LabelBench(bn.ParametrizedSweep):
"""Params must be class-bound for param to assign their names."""
with_units = bn.ResultFloat(units="ms")
no_units = bn.ResultFloat(units="")
unitless = bn.ResultFloat(units="ul")
none_units = bn.ResultFloat(units=None)class TestLabelWithUnits(unittest.TestCase):
def test_name_and_units(self):
self.assertEqual(label_with_units(_LabelBench.param.with_units), "with_units [ms]")
def test_no_units(self):
self.assertEqual(label_with_units(_LabelBench.param.no_units), "no_units")
def test_unitless_convention(self):
"""'ul' is the sweep-variable convention for unitless and must not be shown."""
self.assertEqual(label_with_units(_LabelBench.param.unitless), "unitless")
def test_object_without_units_attribute(self):
class Dummy:
name = "without_units"
self.assertEqual(label_with_units(Dummy()), "without_units")
def test_none_units(self):
"""Result vars with units=None should also yield just the name."""
self.assertEqual(label_with_units(_LabelBench.param.none_units), "none_units")Address Sourcery review on #963: - Exercise _to_line_tap_ds via to_line(use_tap=True), asserting the same xlabel/ylabel units as the float-x path - Add label_with_units tests for objects with no units attribute and units=None
Owner
Author
|
Addressed the Sourcery review in a08c472:
On the two non-blocking suggestions, no change made:
|
Performance Report for
|
| Metric | Value |
|---|---|
| Total tests | 1668 |
| Total time | 121.61s |
| Mean | 0.0729s |
| Median | 0.0020s |
Top 10 slowest tests
| Test | Time (s) |
|---|---|
test.test_bench_examples.TestBenchExamples::test_example_meta |
17.362 |
test.test_over_time_save_perf::test_save_faster_without_aggregated_tab |
4.622 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_drift.py] |
4.336 |
test.test_split_render_examples::test_split_render_subprocess_media |
3.071 |
test.test_generated_examples::test_generated_example[cartesian_animation/example_cartesian_animation.py] |
3.032 |
test.test_hash_persistent.TestCrossProcessDeterminism::test_hash_stable_across_two_processes[ResultBool] |
2.917 |
test.test_over_time_repeats.TestMaxSliderPoints::test_default_subsampling_caps_at_max |
2.770 |
test.test_generated_examples::test_generated_example[result_types/result_image/example_result_image_to_video.py] |
2.709 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_noise.py] |
2.589 |
test.test_generated_examples::test_generated_example[regression/example_regression_tuning_step.py] |
2.256 |
Updated by Performance Tracking workflow
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #960, as promised in this comment: the audit found that
curve_result.py,line_result.pyandscatter_result.pywere the remaining plots that didn't show units on their axes (they set no axis label at all).label_with_units()helper inbencher.utilsproducingname [units], following the existing'ul'= unitless convention fromsweep_base.describe_variableHoloviewResult._build_curve_overlay— coversto_curveand the aggregated line/over-time path (x = float input var, y = result var)LineResult.to_line_ds(float-x path and 0D over_time path) and_to_line_tap_dsScatterResult._to_scatter_dskwargs.setdefault, so explicit userxlabel/ylabelkwargs still take precedenceTest plan
test/test_axis_units.py: assertsdistance [m]/throughput [ops/s]on curve and line axes,score [m]on scatter, and unit tests forlabel_with_units(units / no units /'ul')🤖 Generated with Claude Code
Summary by Sourcery
Add shared axis labeling helper and ensure curve, line, and scatter plots display variable units on their axes.
New Features:
Enhancements:
Tests: