Skip to content

Commit

Permalink
fix: Remove icon from rich representation to have a cleaner display (#…
Browse files Browse the repository at this point in the history
…1115)

This PR is removing the icon used in the different `help` method such
that we don't have issue with spacing.
#1104 will provide a richer
output where we can leverage those types of icons instead to purely rely
on textual that are grant in terminal mainly.
  • Loading branch information
glemaitre authored Jan 14, 2025
1 parent 418b72c commit 6a68814
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 33 deletions.
15 changes: 5 additions & 10 deletions skore/src/skore/sklearn/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def _rich_repr(self, class_name, help_method_name):
class _BaseReport(_HelpMixin):
def _get_help_panel_title(self):
return (
f"[bold cyan]:notebook: Tools to diagnose estimator "
f"[bold cyan]Tools to diagnose estimator "
f"{self.estimator_name}[/bold cyan]"
)

Expand Down Expand Up @@ -132,9 +132,7 @@ def _create_help_tree(self):
# Add accessor methods first
for accessor_attr, config in self._ACCESSOR_CONFIG.items():
accessor = getattr(self, accessor_attr)
branch = tree.add(
f"[bold cyan].{config['name']} {config['icon']}[/bold cyan]"
)
branch = tree.add(f"[bold cyan].{config['name']}[/bold cyan]")

# Add main accessor methods first
methods = accessor._get_methods_for_help()
Expand All @@ -149,9 +147,7 @@ def _create_help_tree(self):
# Add sub-accessors after main methods
for sub_attr, sub_obj in inspect.getmembers(accessor):
if isinstance(sub_obj, _BaseAccessor) and not sub_attr.startswith("_"):
sub_branch = branch.add(
f"[bold cyan].{sub_attr} {sub_obj._icon}[/bold cyan]"
)
sub_branch = branch.add(f"[bold cyan].{sub_attr}[/bold cyan]")

# Add sub-accessor methods
sub_methods = sub_obj._get_methods_for_help()
Expand Down Expand Up @@ -183,13 +179,12 @@ def _create_help_tree(self):
class _BaseAccessor(_HelpMixin):
"""Base class for all accessors."""

def __init__(self, parent, icon):
def __init__(self, parent):
self._parent = parent
self._icon = icon

def _get_help_panel_title(self):
name = self.__class__.__name__.replace("_", "").replace("Accessor", "").lower()
return f"{self._icon} Available {name} methods"
return f"Available {name} methods"

def _create_help_tree(self):
"""Create a rich Tree with the available methods."""
Expand Down
3 changes: 1 addition & 2 deletions skore/src/skore/sklearn/_base.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ class _HelpMixin:

class _BaseAccessor(_HelpMixin):
_parent: Any
_icon: str

def __init__(self, parent: Any, icon: str) -> None: ...
def __init__(self, parent: Any) -> None: ...
def _get_help_panel_title(self) -> str: ...
def _create_help_tree(self) -> Tree: ...
def _get_X_y_and_data_source_hash(
Expand Down
10 changes: 5 additions & 5 deletions skore/src/skore/sklearn/_estimator/metrics_accessor.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class _MetricsAccessor(_BaseAccessor, DirNamesMixin):
}

def __init__(self, parent):
super().__init__(parent, icon=":straight_ruler:")
super().__init__(parent)

# TODO: should build on the `add_scorers` function
def report_metrics(
Expand Down Expand Up @@ -797,7 +797,7 @@ def _create_help_tree(self):
tree = super()._create_help_tree()

# Add plot methods in a separate branch
plot_branch = tree.add("[bold cyan].plot :art:[/bold cyan]")
plot_branch = tree.add("[bold cyan].plot[/bold cyan]")
plot_methods = self.plot._get_methods_for_help()
plot_methods = self.plot._sort_methods_for_help(plot_methods)

Expand All @@ -809,7 +809,7 @@ def _create_help_tree(self):
return tree

def _get_help_panel_title(self):
return f"[bold cyan]{self._icon} Available metrics methods[/bold cyan]"
return "[bold cyan]Available metrics methods[/bold cyan]"

def _get_help_legend(self):
return (
Expand Down Expand Up @@ -837,7 +837,7 @@ class _PlotMetricsAccessor(_BaseAccessor):
"""Plotting methods for the metrics accessor."""

def __init__(self, parent):
super().__init__(parent._parent, icon=":art:")
super().__init__(parent._parent)
self._metrics_parent = parent

def _get_display(
Expand Down Expand Up @@ -1099,7 +1099,7 @@ def prediction_error(
)

def _get_help_panel_title(self):
return f"[bold cyan]{self._icon} Available plot methods[/bold cyan]"
return "[bold cyan]Available plot methods[/bold cyan]"

def _get_help_tree_title(self):
return "[bold cyan]reporter.metrics.plot[/bold cyan]"
Expand Down
5 changes: 1 addition & 4 deletions skore/src/skore/sklearn/_estimator/report.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,7 @@ class EstimatorReport(_BaseReport, DirNamesMixin):
"""

_ACCESSOR_CONFIG = {
"metrics": {"icon": ":straight_ruler:", "name": "metrics"},
# Add other accessors as they're implemented
# "inspection": {"icon": ":magnifying_glass:", "name": "inspection"},
# "linting": {"icon": ":check:", "name": "linting"},
"metrics": {"name": "metrics"},
}

@staticmethod
Expand Down
2 changes: 1 addition & 1 deletion skore/src/skore/sklearn/_plot/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def _create_help_panel(self):
return Panel(
self._create_help_tree(),
title=(
f"[bold cyan]:bar_chart: {self.__class__.__name__} for "
f"[bold cyan]{self.__class__.__name__} for "
f"{self.estimator_name}[/bold cyan]"
),
border_style="orange1",
Expand Down
2 changes: 1 addition & 1 deletion skore/tests/unit/sklearn/plot/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def test_display_help(pyplot, capsys, plot_func, estimator, dataset):

display.help()
captured = capsys.readouterr()
assert f"📊 {display.__class__.__name__}" in captured.out
assert f"{display.__class__.__name__}" in captured.out


@pytest.mark.parametrize(
Expand Down
10 changes: 5 additions & 5 deletions skore/tests/unit/sklearn/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ def test_base_accessor_get_X_y_and_data_source_hash_error():

estimator = LogisticRegression().fit(X_train, y_train)
report = MockReport(estimator, X_train=None, y_train=None, X_test=None, y_test=None)
accessor = _BaseAccessor(parent=report, icon="")
accessor = _BaseAccessor(parent=report)

err_msg = re.escape(
"Invalid data source: unknown. Possible values are: " "test, train, X_y."
Expand All @@ -234,7 +234,7 @@ def test_base_accessor_get_X_y_and_data_source_hash_error():
report = MockReport(
estimator, X_train=X_train, y_train=y_train, X_test=X_test, y_test=y_test
)
accessor = _BaseAccessor(parent=report, icon="")
accessor = _BaseAccessor(parent=report)

for data_source in ("train", "test"):
err_msg = f"X and y must be None when data_source is {data_source}."
Expand All @@ -251,13 +251,13 @@ def test_base_accessor_get_X_y_and_data_source_hash_error():
# use `custom_metric` for them.
estimator = KMeans(n_clusters=2).fit(X_train)
report = MockReport(estimator, X_test=X_test)
accessor = _BaseAccessor(parent=report, icon="")
accessor = _BaseAccessor(parent=report)
err_msg = "X must be provided."
with pytest.raises(ValueError, match=err_msg):
accessor._get_X_y_and_data_source_hash(data_source="X_y")

report = MockReport(estimator)
accessor = _BaseAccessor(parent=report, icon="")
accessor = _BaseAccessor(parent=report)
for data_source in ("train", "test"):
err_msg = re.escape(
f"No {data_source} data (i.e. X_{data_source}) were provided when "
Expand All @@ -279,7 +279,7 @@ def test_base_accessor_get_X_y_and_data_source_hash(data_source):
report = MockReport(
estimator, X_train=X_train, y_train=y_train, X_test=X_test, y_test=y_test
)
accessor = _BaseAccessor(parent=report, icon="")
accessor = _BaseAccessor(parent=report)
kwargs = {"X": X_test, "y": y_test} if data_source == "X_y" else {}
X, y, data_source_hash = accessor._get_X_y_and_data_source_hash(
data_source=data_source, **kwargs
Expand Down
8 changes: 3 additions & 5 deletions skore/tests/unit/sklearn/test_estimator.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,7 @@ def test_estimator_report_help(capsys, binary_classification_data):

report.help()
captured = capsys.readouterr()
assert (
f"📓 Tools to diagnose estimator {estimator.__class__.__name__}" in captured.out
)
assert f"Tools to diagnose estimator {estimator.__class__.__name__}" in captured.out


def test_estimator_report_repr(binary_classification_data):
Expand Down Expand Up @@ -336,7 +334,7 @@ def test_estimator_report_plot_help(capsys, binary_classification_data):

report.metrics.plot.help()
captured = capsys.readouterr()
assert "🎨 Available plot methods" in captured.out
assert "Available plot methods" in captured.out


def test_estimator_report_plot_repr(binary_classification_data):
Expand Down Expand Up @@ -476,7 +474,7 @@ def test_estimator_report_metrics_help(capsys, binary_classification_data):

report.metrics.help()
captured = capsys.readouterr()
assert "📏 Available metrics methods" in captured.out
assert "Available metrics methods" in captured.out


def test_estimator_report_metrics_repr(binary_classification_data):
Expand Down

0 comments on commit 6a68814

Please sign in to comment.