Add "↑/↓ better" direction annotations to all metric y-axes - #3
Merged
Conversation
Closed
Agent-Logs-Url: https://github.com/lamalab-org/clever-materials-hans/sessions/abebaf5d-1e38-4f76-9acf-46d173a30d6c Co-authored-by: kjappelbaum <32935233+kjappelbaum@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Update axes for consistent interpretation in plots
Add "↑/↓ better" direction annotations to all metric y-axes
Apr 2, 2026
kjappelbaum
approved these changes
Apr 12, 2026
kjappelbaum
marked this pull request as ready for review
April 12, 2026 06:04
Reviewer's GuideAdds a reusable helper to annotate metric plots with "↑/↓ better" indicators and applies it consistently to all metric y-axes across plotting routines so readers can immediately see whether higher or lower values are better for each metric. Class diagram for plotting_utils metric direction helpers and usageclassDiagram
class MetricDirectionHelper {
<<module_helpers>>
_LOWER_IS_BETTER_METRICS : set
_is_lower_better(metric str) bool
_add_good_direction_annotation(ax plt_Axes, metric str, compact bool) void
}
class PlotPerformanceComparison {
plot_performance_comparison(results Dict_str_Any, target_type str) None
}
class PlotParameterSweepResults {
plot_parameter_sweep_results(sweep_results List_Dict_str_Any, ...) None
}
class PlotMetaPredictionPanel {
_plot_meta_prediction_panel(ax plt_Axes, results Dict_str_Any, meta_type str, metric str, compact bool) None
}
class CreateMainFigurePanel {
create_main_figure_panel(results Dict_str_Any, main_metric str, ...) None
}
class PlotMetaComparison {
plot_meta_comparison(comparison_results Dict_str_Any, primary_metric str, ...) None
}
MetricDirectionHelper <|.. PlotPerformanceComparison : uses
MetricDirectionHelper <|.. PlotParameterSweepResults : uses
MetricDirectionHelper <|.. PlotMetaPredictionPanel : uses
MetricDirectionHelper <|.. CreateMainFigurePanel : uses
MetricDirectionHelper <|.. PlotMetaComparison : uses
PlotPerformanceComparison ..> MetricDirectionHelper : calls _add_good_direction_annotation
PlotParameterSweepResults ..> MetricDirectionHelper : calls _add_good_direction_annotation
PlotMetaPredictionPanel ..> MetricDirectionHelper : calls _add_good_direction_annotation
CreateMainFigurePanel ..> MetricDirectionHelper : calls _add_good_direction_annotation
PlotMetaComparison ..> MetricDirectionHelper : calls _add_good_direction_annotation
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Currently any metric not in
_LOWER_IS_BETTER_METRICSis treated as↑ better; consider either making the mapping explicit for all expected metrics or skipping/raising for unknown names so you don’t silently mislabel new or custom metrics. - You might want an optional flag on the public plotting functions (e.g.,
show_direction_annotations: bool = True) that is passed into_add_good_direction_annotationso callers can disable these labels when they conflict with tight layouts or custom figure styling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Currently any metric not in `_LOWER_IS_BETTER_METRICS` is treated as `↑ better`; consider either making the mapping explicit for all expected metrics or skipping/raising for unknown names so you don’t silently mislabel new or custom metrics.
- You might want an optional flag on the public plotting functions (e.g., `show_direction_annotations: bool = True`) that is passed into `_add_good_direction_annotation` so callers can disable these labels when they conflict with tight layouts or custom figure styling.
## Individual Comments
### Comment 1
<location path="src/scripts/plotting_utils.py" line_range="64-66" />
<code_context>
+_LOWER_IS_BETTER_METRICS = {'mae', 'mape', 'rmse', 'mse'}
+
+
+def _is_lower_better(metric: str) -> bool:
+ """Return True if lower values are better for this metric."""
+ return metric.lower() in _LOWER_IS_BETTER_METRICS
+
+
</code_context>
<issue_to_address>
**suggestion:** Normalize metric strings more defensively to avoid subtle mismatches.
`_is_lower_better` currently requires an exact lowercase match. If callers pass names with whitespace or minor decoration (e.g. `'rmse '`, `'RMSE\n'`, `'rmse (val)'`), it will incorrectly fall back to higher-is-better. Consider normalizing more aggressively (e.g. `metric.strip().lower()`) and, if relevant, handling simple aliases/suffixes before the membership check to avoid wrong arrow directions from small naming differences.
```suggestion
def _is_lower_better(metric: str) -> bool:
"""Return True if lower values are better for this metric.
Normalizes common formatting variations (whitespace, case, and
simple parenthetical decorations like "rmse (val)").
"""
# Normalize whitespace and case
normalized = metric.strip().lower()
# Drop simple parenthetical decorations, e.g. "rmse (val)" -> "rmse"
if "(" in normalized:
normalized = normalized.split("(", 1)[0].rstrip()
return normalized in _LOWER_IS_BETTER_METRICS
```
</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
+64
to
+66
| def _is_lower_better(metric: str) -> bool: | ||
| """Return True if lower values are better for this metric.""" | ||
| return metric.lower() in _LOWER_IS_BETTER_METRICS |
There was a problem hiding this comment.
suggestion: Normalize metric strings more defensively to avoid subtle mismatches.
_is_lower_better currently requires an exact lowercase match. If callers pass names with whitespace or minor decoration (e.g. 'rmse ', 'RMSE\n', 'rmse (val)'), it will incorrectly fall back to higher-is-better. Consider normalizing more aggressively (e.g. metric.strip().lower()) and, if relevant, handling simple aliases/suffixes before the membership check to avoid wrong arrow directions from small naming differences.
Suggested change
| def _is_lower_better(metric: str) -> bool: | |
| """Return True if lower values are better for this metric.""" | |
| return metric.lower() in _LOWER_IS_BETTER_METRICS | |
| def _is_lower_better(metric: str) -> bool: | |
| """Return True if lower values are better for this metric. | |
| Normalizes common formatting variations (whitespace, case, and | |
| simple parenthetical decorations like "rmse (val)"). | |
| """ | |
| # Normalize whitespace and case | |
| normalized = metric.strip().lower() | |
| # Drop simple parenthetical decorations, e.g. "rmse (val)" -> "rmse" | |
| if "(" in normalized: | |
| normalized = normalized.split("(", 1)[0].rstrip() | |
| return normalized in _LOWER_IS_BETTER_METRICS |
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
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.
Mixed metrics across subplots (MAE lower-is-better vs. F1/Accuracy higher-is-better) make figures hard to scan, especially since MAE panels visually trend in the opposite direction to the others.
Changes
New helpers in
plotting_utils.py_LOWER_IS_BETTER_METRICSconstant set (mae,mape,rmse,mse)_is_lower_better(metric)— classifies metric direction_add_good_direction_annotation(ax, metric, compact)— places a small italic gray↓ betteror↑ betterlabel at the top-left of each axisApplied to every metric y-axis across all plotting functions:
plot_performance_comparison()— each metric subplotplot_parameter_sweep_results()— each parameter-sweep panel_plot_meta_prediction_panel()— author/journal (F1 →↑) and year (MAE →↓)create_main_figure_panel()— bottom performance panelplot_meta_comparison()— property prediction panelSummary by Sourcery
Annotate all metric plots with visual cues indicating whether higher or lower values are better.
Enhancements: