Support target_names in classification metrics renderers#1841
Support target_names in classification metrics renderers#1841Br1an67 wants to merge 1 commit intoevidentlyai:mainfrom
Conversation
Add target_names support to ClassificationClassSeparationPlot, ClassificationProbDistribution, and ClassificationQualityByFeatureTable renderers. When column_mapping.target_names is set as a dict, the human-readable names are now used in plot labels, tab titles, and legend entries instead of raw class values. Fixes evidentlyai#578
Nik-Reddy
left a comment
There was a problem hiding this comment.
Nice work tracking down all three renderers that were missing target_names support — the pattern matches what ClassificationQualityByClass already does, so the approach is solid.
A few things I noticed:
1. Potential bug in _resolve_target_name — operator precedence
The or/ternary logic in the resolver is tricky. This line:
resolved = target_names.get(label) or target_names.get(int(label)) if isinstance(label, str) else target_names.get(label)
If the mapped value is falsy (e.g., 0 or empty string), the \or\ will skip it and try int(label) instead. Also if label is something like \\foo\\ that can't be cast to int, this raises a ValueError. Might be worth an explicit guard:
if isinstance(label, str):
resolved = target_names.get(label)
if resolved is None:
try:
resolved = target_names.get(int(label))
except (ValueError, TypeError):
pass
else:
resolved = target_names.get(label)
2. DRY — _resolve_target_name is copy-pasted 3 times
The function appears identically as a module-level function in class_separation_metric.py, a staticmethod in ClassificationProbDistributionRenderer, and another staticmethod in ClassificationQualityByFeatureTableRenderer. Could you pull it into a shared utility so if the logic needs fixing it only needs changing once?
3. No tests
Even a small unit test for _resolve_target_name covering the dict-mapping, int-key fallback, and None (no mapping) cases would go a long way toward preventing regressions.
4. Minor: target_name vs target_names naming
Having both target_name: str and target_names: Optional[TargetNames] on the same result class is a bit confusing — a docstring clarifying that target_name is the column name and target_names is the label-to-display mapping would help future readers.
Nik-Reddy
left a comment
There was a problem hiding this comment.
Did another pass on this. A couple of spots where the rendered output might still show raw class values even after this change.
|
|
||
| def _resolve_target_name(label, target_names: Optional[TargetNames]) -> str: | ||
| if target_names is not None and isinstance(target_names, dict): | ||
| resolved = target_names.get(label) or target_names.get(int(label)) if isinstance(label, str) else target_names.get(label) # type: ignore[arg-type] |
There was a problem hiding this comment.
Two things here. First, this only triggers when target_names is a dict, but TargetNames is typed as Union[List, Dict]. If someone passes a list, the resolver does nothing and falls through to str(label). Not sure if list-based target_names is actually used in practice, but worth checking.
Second, the or on this line can swallow falsy mapped values. If target_names maps a label to 0 or an empty string, the or skips it and tries int(label) instead. And if label is a non-numeric string like "cat", int(label) raises ValueError. Safer to check is None explicitly rather than relying on truthiness.
| color_options=self.color_options, | ||
| ) | ||
| tabs = [TabData(name, widget) for name, widget in tab_data] | ||
| tabs = [TabData(_resolve_target_name(name, metric_result.target_names), widget) for name, widget in tab_data] |
There was a problem hiding this comment.
Tab names are resolved here which is good, but the inner traces built by class_separation_traces_raw and class_separation_traces_agg still use str(label) for name and legendgroup. So the tab heading will show the display name but the plot legend inside the tab still shows the raw class value. Might want to pass target_names into those trace-building helpers too.
| class ClassificationQualityByFeatureTableRenderer(MetricRenderer): | ||
| @staticmethod | ||
| def _resolve_target_name(label, target_names: Optional[TargetNames]) -> str: | ||
| if target_names is not None and isinstance(target_names, dict): |
There was a problem hiding this comment.
Same resolver function as the other two files. If the operator precedence or dict-only issue needs fixing, it has to be done in three places. Pulling this into something like evidently.legacy.utils or a shared base would keep it maintainable.
Fixes #578
When
target_namesis specified incolumn_mappingas a dict mapping raw class values to human-readable names, three classification metric renderers were ignoring it and displaying raw class values. This PR addstarget_namessupport to:The existing
ClassificationQualityByClassmetric already supportedtarget_names— the same pattern is applied here.Changes:
target_names: Optional[TargetNames]field to each result class (defaults toNonefor backward compatibility)dataset_columns.target_namesthrough the calculate → result → renderer pipeline_resolve_target_name()helper that maps raw labels to display names when a dict mapping is provided