From d72597e54c506e5c41f0495d08f7222edaa01624 Mon Sep 17 00:00:00 2001 From: Jerome Dockes Date: Fri, 19 Sep 2025 23:44:08 +0200 Subject: [PATCH 1/3] show transformed data not dataop itself in fit report --- skrub/_data_ops/_inspection.py | 5 ++++- skrub/_data_ops/tests/test_inspection.py | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/skrub/_data_ops/_inspection.py b/skrub/_data_ops/_inspection.py index 1812a3c48..5363fe5c5 100644 --- a/skrub/_data_ops/_inspection.py +++ b/skrub/_data_ops/_inspection.py @@ -50,7 +50,10 @@ def _use_table_report_display(): def node_report(data_op, mode="preview", environment=None, **report_kwargs): - result = evaluate(data_op, mode=mode, environment=environment) + if mode in data_op._skrub_impl.results: + result = data_op._skrub_impl.results[mode] + else: + result = evaluate(data_op, mode=mode, environment=environment) if sbd.is_column(result): # TODO say in page that it was a column not df # maybe this should be handled by tablereport? or we should have a diff --git a/skrub/_data_ops/tests/test_inspection.py b/skrub/_data_ops/tests/test_inspection.py index 78d677f4b..c11b7987e 100644 --- a/skrub/_data_ops/tests/test_inspection.py +++ b/skrub/_data_ops/tests/test_inspection.py @@ -2,6 +2,7 @@ import re import sys import webbrowser +from pathlib import Path from unittest.mock import Mock import pytest @@ -84,6 +85,25 @@ def test_full_report_failed_apply(): assert report["error"] is not None +@pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz") +def test_report_fit_mode(): + # non-regression: in fit mode the individual node pages used to show the + # dataop itself as the output instead of the result of fit_transform for + # intermediate nodes. + learner = ( + skrub.var("a") + .skb.apply_func(lambda x: x[::-1]) + .skb.apply_func(lambda x: x.upper()) + .skb.make_learner() + ) + report = learner.report(environment={"a": "hello"}, mode="fit", open=False) + report_dir = Path(report["report_path"]).parent + node_1_report = (report_dir / "node_1.html").read_text("utf-8") + # the page does display the actual output rather than the repr of the node + # ('hello'[::-1] = 'olleh') + assert "olleh" in node_1_report + + @pytest.mark.skipif(not _inspection._has_graphviz(), reason="report requires graphviz") def test_full_report_open(monkeypatch): mock = Mock() From b4dfe531bb8b0c89934c18188d26d822356a0b0d Mon Sep 17 00:00:00 2001 From: Jerome Dockes Date: Sat, 20 Sep 2025 00:07:36 +0200 Subject: [PATCH 2/3] changelog --- CHANGES.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index fde50f36a..11bd00bda 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -50,6 +50,9 @@ Bugfixes (not into their subclasses). If you need the items evaluated (ie if they contain DataOps or Choices), store them in one of the builtin collections. :pr:`1612` by :user:`Jérôme Dockès `. +- :meth:`SkrubLearner.report` with ``mode="fit"`` used to display the dataops + themselves, rather than their outputs, in the report. This has been fixed in + :pr:`1623` by :user:`Jérôme Dockès `. Release 0.6.1 From 1b7d663604d2993369d0117a82742337d20c3852 Mon Sep 17 00:00:00 2001 From: Jerome Dockes Date: Sat, 20 Sep 2025 00:17:38 +0200 Subject: [PATCH 3/3] _ --- skrub/_data_ops/_evaluation.py | 7 +++---- skrub/_data_ops/_inspection.py | 5 +---- skrub/_data_ops/tests/test_evaluation.py | 9 +++++++++ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/skrub/_data_ops/_evaluation.py b/skrub/_data_ops/_evaluation.py index 4338cfc53..2083638d6 100644 --- a/skrub/_data_ops/_evaluation.py +++ b/skrub/_data_ops/_evaluation.py @@ -444,10 +444,9 @@ def evaluate(data_op, mode="preview", environment=None, clear=False, callbacks=( else: callbacks = () try: - result = _Evaluator( - mode=mode, environment=environment, callbacks=callbacks - ).run(data_op) - return data_op if mode == "fit" else result + return _Evaluator(mode=mode, environment=environment, callbacks=callbacks).run( + data_op + ) finally: if clear: clear_results(data_op, mode=mode) diff --git a/skrub/_data_ops/_inspection.py b/skrub/_data_ops/_inspection.py index 5363fe5c5..1812a3c48 100644 --- a/skrub/_data_ops/_inspection.py +++ b/skrub/_data_ops/_inspection.py @@ -50,10 +50,7 @@ def _use_table_report_display(): def node_report(data_op, mode="preview", environment=None, **report_kwargs): - if mode in data_op._skrub_impl.results: - result = data_op._skrub_impl.results[mode] - else: - result = evaluate(data_op, mode=mode, environment=environment) + result = evaluate(data_op, mode=mode, environment=environment) if sbd.is_column(result): # TODO say in page that it was a column not df # maybe this should be handled by tablereport? or we should have a diff --git a/skrub/_data_ops/tests/test_evaluation.py b/skrub/_data_ops/tests/test_evaluation.py index 53683ff2d..7774f5e6b 100644 --- a/skrub/_data_ops/tests/test_evaluation.py +++ b/skrub/_data_ops/tests/test_evaluation.py @@ -382,3 +382,12 @@ class Dict(dict): d = Dict() assert skrub.as_data_op(d).skb.eval() is d + + +def test_eval_fit(): + # evaluate() does no special handling of "fit" mode. + # the learner's fit() discards the result and returns self. + dop = skrub.as_data_op(1) + assert _evaluation.evaluate(dop, mode="fit") == 1 + learner = dop.skb.make_learner() + assert learner.fit({}) is learner