diff --git a/src/flightdeck/report/charts.py b/src/flightdeck/report/charts.py index bc1d84e..d0418c8 100644 --- a/src/flightdeck/report/charts.py +++ b/src/flightdeck/report/charts.py @@ -34,10 +34,32 @@ def _nice_top(value: float) -> float: return raw * 10 +def _nice_step(span: float) -> float: + """Smallest 1/2/2.5/5 x 10^k step that cuts span into roughly three intervals.""" + raw = span / 3 + magnitude = 10 ** math.floor(math.log10(raw)) + for mult in (1, 2, 2.5, 5, 10): + if magnitude * mult >= raw: + return magnitude * mult + return magnitude * 10 + + def _fmt(value: float, decimals: int | None = None) -> str: if decimals is None: decimals = 0 if abs(value) >= 100 or value == int(value) else 1 - return f"{value:,.{decimals}f}" + text = f"{value:,.{decimals}f}" + # Never a signed zero. format.money makes the same promise for currency and + # _money for the bar labels; once ticks can go negative, "-0.0 h" would sit on + # the same axis as "0 h" and read as two different zeros. + return text.lstrip("-") if float(text.replace(",", "")) == 0 else text + + +def _coord(value: float) -> str: + """A coordinate rendered at its natural precision: whole numbers keep their + integer form. The zero line lands exactly on the plot floor whenever the + domain floors at zero, and this keeps that emission identical to the + fixed-baseline output it replaced.""" + return str(int(value)) if float(value).is_integer() else f"{value:.1f}" def _money(value: float, unit: str) -> str: @@ -49,17 +71,57 @@ def _money(value: float, unit: str) -> str: return f"{'−' if negative else ''}{unit}{_fmt(abs(value))}" -def _grid_and_axis(top: float, unit: str, plot_h: float) -> str: - parts = [] - for index in range(1, 4): # 3 hairlines + baseline - y = _PAD_T + plot_h * (1 - index / 3) - tick = top * index / 3 - parts.append(f'') - parts.append( - f'{_fmt(tick)}{unit}' - ) - baseline_y = _PAD_T + plot_h - parts.append(f'') +def _tick_decimals(step: float) -> int: + """Decimals enough to tell one tick from the next. _fmt picks its precision from + the value, which collapses two adjacent small ticks to the same label (-0.05 and + -0.10 both render "-0.1"); a tick scale has to be read off the step instead.""" + decimals = max(0, -math.floor(math.log10(step))) + if round(step, decimals) != step: # a 2.5-style step needs one more + decimals += 1 + return min(decimals, 6) + + +def _tick(y: float, value: float, unit: str, decimals: int | None = None, rule: bool = True) -> str: + line = f'' if rule else "" + return ( + f'{line}{_fmt(value, decimals)}{unit}' + ) + + +def _grid_and_axis(top: float, unit: str, plot_h: float, bottom: float = 0.0, step: float = 0.0) -> str: + """Gridlines and the zero axis over the domain [bottom, top]. + + A domain floored at zero keeps the established framing -- three hairlines at + thirds of ``top``, the axis ruling the plot floor where zero is self-evident + and needs no tick. Those charts, column_chart included, render byte-for-byte + as before, with one deliberate exception: a label that rounds to zero loses + its minus sign (see _fmt). + + A two-sided domain instead walks multiples of ``step``. Zero always lands on + a labeled position and so does the domain minimum: without them the axis is + just a heavier rule at an arbitrary height, and a reader can see the dip but + not read how deep it goes. The hairline count is whatever the step yields + (2-4 in practice) rather than a fixed three, because on a two-sided scale the + tick values have to be readable numbers before they are a tidy count.""" + if bottom == 0.0: + parts = [ + _tick(_PAD_T + plot_h * (1 - index / 3), top * index / 3, unit) + for index in range(1, 4) # 3 hairlines + baseline + ] + zero = _coord(_PAD_T + plot_h) + else: + span = top - bottom + decimals = _tick_decimals(step) + parts = [] + for k in range(round(bottom / step), round(top / step) + 1): + value = k * step + y = _PAD_T + plot_h * (1 - (value - bottom) / span) + # Zero's rule is the axis itself, drawn below; a hairline there too + # would put two opaque 1px strokes on the same pixel. + parts.append(_tick(y, value, unit, decimals, rule=bool(k))) + zero = _coord(_PAD_T + plot_h * (1 - (0.0 - bottom) / span)) + parts.append(f'') return "".join(parts) @@ -79,14 +141,30 @@ def line_chart(chart_id: str, labels: list[str], values: list[float], unit: str, return '

no data yet

' plot_h = _H - _PAD_T - _PAD_B plot_w = _W - _PAD_L - _PAD_R - top = _nice_top(max(values)) + # The domain must contain zero AND every value: hours_saved goes negative in a + # rejection-heavy week (docs/metrics.md — a rejected run earns −m minutes), and + # a domain floored at zero maps that week far below the viewBox, where the + # browser clips it and the card silently hides the bad news. Non-negative data + # keeps bottom=0 and step=top/3, i.e. exactly the previous framing. + hi, lo = max(max(values), 0.0), min(min(values), 0.0) + if lo == 0.0: + top, bottom = _nice_top(hi), 0.0 + step = top / 3 + else: + # Snap both ends to multiples of a nice step so zero is itself a gridline. + step = _nice_step(hi - lo) + top, bottom = math.ceil(hi / step) * step, math.floor(lo / step) * step + span = (top - bottom) or 1.0 n = len(values) xs = [_PAD_L + plot_w * (i + 0.5) / n for i in range(n)] - ys = [_PAD_T + plot_h * (1 - v / top) for v in values] + ys = [_PAD_T + plot_h * (1 - (v - bottom) / span) for v in values] baseline_y = _PAD_T + plot_h + zero_y = _coord(_PAD_T + plot_h * (1 - (0.0 - bottom) / span)) line_path = "M" + " L".join(f"{x:.1f} {y:.1f}" for x, y in zip(xs, ys, strict=True)) - area_path = f"{line_path} L{xs[-1]:.1f} {baseline_y} L{xs[0]:.1f} {baseline_y} Z" + # The area washes back to zero, not to the frame floor, so a negative week + # reads as a dip below the axis instead of a full-height fill. + area_path = f"{line_path} L{xs[-1]:.1f} {zero_y} L{xs[0]:.1f} {zero_y} Z" hover = [] slot = plot_w / n @@ -100,7 +178,7 @@ def line_chart(chart_id: str, labels: list[str], values: list[float], unit: str, end_label = f"{_fmt(values[-1], 1)}{unit}" end_x = min(xs[-1] + 8, _W - _PAD_R - 4) return f""" -{_grid_and_axis(top, unit, plot_h)} +{_grid_and_axis(top, unit, plot_h, bottom, step)} diff --git a/tests/test_charts.py b/tests/test_charts.py index 372c9b0..fae05ee 100644 --- a/tests/test_charts.py +++ b/tests/test_charts.py @@ -1,7 +1,30 @@ -"""Chart rendering edge cases — cross-surface consistency of money labels.""" +"""Chart rendering edge cases — cross-surface consistency of money labels, and +the weekly line chart's numeric domain. + +hours_saved per week can be negative (docs/metrics.md: a rejected run earns −m +minutes), and that series feeds line_chart on the dashboard, so its y-domain must +reach below zero and stay readable there. The terminal sparkline survives the same +input by flooring it (test_terminal.py), which a chart with a labeled axis cannot +do: it has to show how deep the week went. +""" + +import re from flightdeck.format import money -from flightdeck.report.charts import hbar_chart +from flightdeck.report.charts import _H, _PAD_B, _PAD_T, column_chart, hbar_chart, line_chart + +# The band a point may legally occupy: outside it the browser clips against the +# viewBox and the value disappears from the page. +_BAND = (_PAD_T, _H - _PAD_B) + + +def _ticks(svg: str) -> list[str]: + return re.findall(r'class="fd-tick"[^>]*>([^<]+ h)<', svg) + + +def _line_ys(svg: str) -> list[float]: + path = re.search(r'class="fd-line" d="([^"]+)"', svg).group(1) + return [float(y) for y in re.findall(r"[ML]-?[\d.]+ (-?[\d.]+)", path)] def test_hbar_negative_matches_the_shared_money_minus_glyph(): @@ -20,3 +43,88 @@ def test_hbar_negative_matches_the_shared_money_minus_glyph(): def test_hbar_positive_and_empty_render(): assert money(3000.0, "EUR") in hbar_chart([("Up", 3000)], "€") # "€3,000" assert "no data yet" in hbar_chart([], "€") + + +def test_line_chart_keeps_a_negative_week_inside_the_viewbox(): + # A rejection-heavy week is exactly the week an executive must see. Before the + # fix the domain was floored at zero, so -20 mapped to y=582 in a 236-tall + # viewBox: clipped away, and the dashboard quietly hid the bad news. + ys = _line_ys(line_chart("hours", ["W27", "W28", "W29"], [10.0, -20.0, 5.0], " h", "hours saved")) + + assert len(ys) == 3 + assert all(_BAND[0] <= y <= _BAND[1] for y in ys), ys + assert ys[1] > ys[0] and ys[1] > ys[2] # the negative week sits lowest (SVG y grows downward) + + +def test_line_chart_all_negative_stays_in_band_and_hangs_below_the_axis(): + svg = line_chart("hours", ["W27", "W28"], [-5.0, -10.0], " h", "hours saved") + ys = _line_ys(svg) + + assert all(_BAND[0] <= y <= _BAND[1] for y in ys), ys + # Every point is below the zero axis, which is now drawn at the top of the band. + zero_y = float(re.search(r'class="fd-axis"[^/]*y1="([\d.]+)"', svg).group(1)) + assert all(y > zero_y for y in ys) + + +def test_line_chart_non_negative_output_is_byte_identical(): + # Regression guard, pinned against the pre-fix output: widening the domain must + # not move — or reformat — anything on the non-negative path. The axis and the + # area are the two marks the fix actually rewrote, so assert those verbatim + # rather than only the line coordinates. + svg = line_chart("hours", ["a", "b", "c"], [1.0, 2.0, 3.0], " h", "s") + + assert _line_ys(svg) == [168.4, 130.8, 93.2] + assert '' in svg + assert '' in svg + + flat = line_chart("hours", ["a", "b"], [0.0, 0.0], " h", "s") + assert _line_ys(flat) == [206.0, 206.0] + assert '' in flat + + +def test_column_chart_is_untouched_by_the_line_chart_domain_fix(): + # column_chart shares _grid_and_axis with line_chart. It plots AI spend, which + # is never negative, so it must come out exactly as it did before the domain + # became two-sided — the fix has no business changing the spend card. + svg = column_chart(["a", "b", "c"], [10.0, 20.0, 30.0], "€", "AI spend") + + # The zero line keeps its integer form; the bars' own ":.1f" coordinates are + # untouched by the fix and stay as they were. + assert '' in svg + + +def test_line_chart_labels_zero_and_the_depth_of_a_bad_week(): + # Keeping the negative week on the page is only half the fix: without a labeled + # zero the axis is a heavier rule at an arbitrary height, and the reader can see + # the dip but not read how far it goes. + svg = line_chart("hours", ["W27", "W28", "W29"], [10.0, -20.0, 5.0], " h", "hours saved") + ticks = _ticks(svg) + + assert "0 h" in ticks # zero is on the scale, not implied by the frame + assert "-20 h" in ticks # so is the floor of the domain: the depth of the week + values = [float(t.removesuffix(" h")) for t in ticks] + assert values == sorted(values) # monotonic, emitted bottom of the band upward + assert len(set(ticks)) == len(ticks) # every label distinguishable from its neighbour + + +def test_line_chart_ticks_never_render_a_signed_zero(): + # format.money and charts._money both promise a value that rounds to zero is + # unsigned. Ticks only started going negative with the two-sided domain, so the + # same promise has to hold here: "-0.0 h" beside "0.0 h" reads as two zeros. + ticks = _ticks(line_chart("hours", ["W27", "W28"], [0.0, -0.1], " h", "hours saved")) + + assert not [t for t in ticks if t.startswith("-") and float(t.removesuffix(" h")) == 0], ticks + assert len(set(ticks)) == len(ticks), ticks # decimals come off the step, not the value + + +def test_line_chart_draws_no_hairline_under_the_zero_axis(): + # The axis already rules zero; a gridline at the same y would stack two opaque + # 1px strokes and cost the chart one of its three promised hairlines. + svg = line_chart("hours", ["W27", "W28"], [-5.0, -10.0], " h", "hours saved") + + # Compare numbers, not strings: the axis goes through _coord ("18") while + # gridlines use ":.1f" ("18.0"), so a string test silently passes at exactly + # the two integral positions where a regression would land. + axis_y = float(re.search(r'class="fd-axis" x1="44" y1="([\d.]+)"', svg).group(1)) + grid_ys = [float(y) for y in re.findall(r'class="fd-grid" x1="44" y1="([\d.]+)"', svg)] + assert axis_y not in grid_ys, (axis_y, grid_ys)