Add deterministic multi-family admissibility SVG rendering#139
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a Python script to generate SVG visualizations of multi-family admissibility degradation curves, accompanied by a new npm script and comprehensive unit tests. The reviewer identified a potential KeyError when processing incomplete data sets and recommended refactoring the coordinate logic to handle missing points more gracefully. Additionally, feedback was provided regarding the legend's fixed vertical layout, which may cause overflow issues as the number of data families grows.
| legend_height = 30 + 18 * len(families) | ||
| elements.append(f' <rect x="{legend_x}" y="{legend_y}" width="270" height="{legend_height}" fill="#f8f8f8" stroke="#cccccc"/>') |
There was a problem hiding this comment.
The legend height calculation and placement do not account for the total SVG height. If the number of families grows significantly (e.g., beyond 18-20), the legend box will overlap with the X-axis labels or extend beyond the bottom margin of the SVG (560px). Consider implementing a multi-column legend or adding a check to ensure the legend fits within the available vertical space.
| points_by_level = { | ||
| _level_from_fixture_id(point["fixture_id"]): point | ||
| for point in family["curve"]["points"] | ||
| } | ||
|
|
||
| polyline = " ".join( | ||
| f"{_fmt(x_by_level[level])},{_fmt(MARGIN_TOP + ((1.0 - float(points_by_level[level]['overall_admissibility_score'])) * plot_height))}" | ||
| for level in LEVELS | ||
| ) | ||
| elements.append(f' <polyline points="{polyline}" fill="none" stroke="{color}" stroke-width="3"/>') | ||
|
|
||
| for level in LEVELS: | ||
| score = float(points_by_level[level]["overall_admissibility_score"]) | ||
| x = x_by_level[level] | ||
| y = MARGIN_TOP + ((1.0 - score) * plot_height) | ||
| elements.append(f' <circle cx="{_fmt(x)}" cy="{_fmt(y)}" r="4" fill="{color}"/>') |
There was a problem hiding this comment.
The rendering logic assumes that every family contains a data point for every level defined in LEVELS. If a point is missing for a specific level, the script will raise a KeyError on lines 102 and 108. Additionally, the Y-coordinate calculation is duplicated. It is safer to pre-calculate the coordinates and handle missing points gracefully.
| points_by_level = { | |
| _level_from_fixture_id(point["fixture_id"]): point | |
| for point in family["curve"]["points"] | |
| } | |
| polyline = " ".join( | |
| f"{_fmt(x_by_level[level])},{_fmt(MARGIN_TOP + ((1.0 - float(points_by_level[level]['overall_admissibility_score'])) * plot_height))}" | |
| for level in LEVELS | |
| ) | |
| elements.append(f' <polyline points="{polyline}" fill="none" stroke="{color}" stroke-width="3"/>') | |
| for level in LEVELS: | |
| score = float(points_by_level[level]["overall_admissibility_score"]) | |
| x = x_by_level[level] | |
| y = MARGIN_TOP + ((1.0 - score) * plot_height) | |
| elements.append(f' <circle cx="{_fmt(x)}" cy="{_fmt(y)}" r="4" fill="{color}"/>') | |
| coords: list[tuple[float, float]] = [] | |
| for level in LEVELS: | |
| point = points_by_level.get(level) | |
| if not point: | |
| continue | |
| score = float(point["overall_admissibility_score"]) | |
| x = x_by_level[level] | |
| y = MARGIN_TOP + ((1.0 - score) * plot_height) | |
| coords.append((x, y)) | |
| if not coords: | |
| continue | |
| polyline_pts = " ".join(f"{_fmt(cx)},{_fmt(cy)}" for cx, cy in coords) | |
| elements.append(f' <polyline points="{polyline_pts}" fill="none" stroke="{color}" stroke-width="3"/>') | |
| for cx, cy in coords: | |
| elements.append(f' <circle cx="{_fmt(cx)}" cy="{_fmt(cy)}" r="4" fill="{color}"/>') |
Motivation
Description
scripts/render_multi_family_admissibility_svg.pythat readsartifacts/multi_family_admissibility_results.json, sorts families lexicographically, maps fixture IDs to the four degradation levels (baseline,mild,moderate,severe), and emits a plain SVG with fixed dimensions, stable element ordering, fixed numeric formatting, and a hardcoded color palette.artifacts/multi_family_admissibility_curves.svgcontaining one polyline + point per family plus axes, labels, and a deterministic legend.tests/test_multi_family_svg_renderer.pythat assert determinism, artifact parity, presence of both current families, presence of all four degradation levels, and absence of nondeterministic tokens.generate:multi-family-svgthat runspython scripts/render_multi_family_admissibility_svg.pyfor deterministic regeneration.Testing
python scripts/render_multi_family_admissibility_svg.pyto generate the artifact and verify output was produced successfully.pytest tests/test_multi_family_svg_renderer.py -qand observed5 passedfor the new renderer tests.pytest tests/test_artifact_reproducibility.py -qand observed1 passedfor artifact reproducibility checks.npm run checkwhich completed the repository checks and test suite with218 passedin the run reported locally.Codex Task