005 distribution trace visual#17
Conversation
- Added support for visualizing distribution traces with histogram and heatmap outputs. - Updated command help to include new usage examples for distribution tracing. - Implemented step selection for distribution traces, allowing users to specify exact or nearest steps. - Enhanced tests to cover new distribution visualization features and ensure correct behavior across various output formats. - Updated `.gitignore` to include additional temporary files related to distribution tracing.
…t practices - Added a new section on "Fast Recipes" in SKILL.md for common analysis tasks. - Introduced detailed analysis workflows and critical rules for using aimx effectively. - Included guidelines for ranking runs, inspecting curve health, and compact local analysis. - Enhanced aimx-cli.md with analysis patterns and examples for comparing runs and detecting anomalies. - Emphasized the importance of structured evidence and compact reporting in analysis.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the aimx trace distribution command, enabling users to visualize Aim distribution sequences as terminal-based histograms and heatmaps. The changes include a refactored trace command pipeline, new rendering logic using Rich for blue-gradient visuals, and extensive documentation updates covering analysis patterns and workflows. The review feedback correctly identifies several edge cases in the new rendering code, specifically regarding the handling of non-finite values (NaN/Inf) and a potential division-by-zero error when the terminal height is configured to a very small value.
|
|
||
|
|
||
| def _sample_points_for_height(points: list[Any], max_rows: int) -> list[Any]: | ||
| if max_rows <= 0 or len(points) <= max_rows: |
There was a problem hiding this comment.
If max_rows is 1 and len(points) > 1, the calculation of indexes on line 171 will raise a ZeroDivisionError because of max_rows - 1. This can be triggered by a user providing a small --height value (e.g., 12 or 13). The guard should be updated to handle max_rows <= 1.
| if max_rows <= 0 or len(points) <= max_rows: | |
| if max_rows <= 1 or len(points) <= max_rows: |
| start = index * len(values) // width | ||
| end = (index + 1) * len(values) // width | ||
| bucket = values[start:end] or [values[start]] | ||
| compressed.append(max(bucket)) |
There was a problem hiding this comment.
If a bucket contains non-finite values (NaN or Inf), max(bucket) may return them, which can cause issues during rendering in _intensity_text. It is safer to filter for finite values and provide a default.
| compressed.append(max(bucket)) | |
| compressed.append(max((v for v in bucket if math.isfinite(v)), default=0.0)) |
| text = Text() | ||
| if not values: | ||
| return text | ||
| max_value = max(float(v) for v in values) |
There was a problem hiding this comment.
If values contains non-finite values (NaN or Inf), max() will return them. This can lead to a ValueError when round() is called on a NaN scale later in the function. Filtering for finite values ensures robustness against unexpected data in the Aim repository.
finite_values = [float(v) for v in values if math.isfinite(v)]
max_value = max(finite_values, default=0.0)| return text | ||
| for value in values: | ||
| if value <= 0: | ||
| text.append(_DISTRIBUTION_BLOCKS[0], style=_DISTRIBUTION_ZERO_STYLE) |
There was a problem hiding this comment.
Non-finite values should be treated as zero or skipped to avoid crashes during scale calculation and rounding.
| text.append(_DISTRIBUTION_BLOCKS[0], style=_DISTRIBUTION_ZERO_STYLE) | |
| if not math.isfinite(value) or value <= 0: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c5ef4d329
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if max_rows <= 0 or len(points) <= max_rows: | ||
| return points | ||
| indexes = sorted({round(i * (len(points) - 1) / (max_rows - 1)) for i in range(max_rows)}) |
There was a problem hiding this comment.
When --height is small (e.g., 12 or 13), render_distribution_visual computes max_heatmap_rows as 1; if the selected series has more than one point, _sample_points_for_height reaches the index calculation and divides by (max_rows - 1), which is zero. This raises ZeroDivisionError and makes aimx trace distribution ... --height <small> fail instead of rendering a reduced heatmap, so the sampler should special-case max_rows == 1 before the interpolation formula.
Useful? React with 👍 / 👎.
1 participant
No description provided.