feat: Add the data accessor to CrossValidationReport #1965

MarieSacksick · 2025-08-04T16:35:51Z

Closes #1964

On the way, I split the tests of the data accessor for estimator report between things specifics and common.

github-actions · 2025-08-04T16:46:10Z

Coverage Report for skore/

File	Stmts	Miss	Cover	Missing
skore/src/skore
__init__.py	23	0	100%
_config.py	31	0	100%
exceptions.py	4	4	0%	4, 15, 19, 23
skore/src/skore/_sklearn
__init__.py	6	0	100%
_base.py	198	14	92%	45, 58, 127, 130, 183, 186–187, 189–192, 225, 228–229
find_ml_task.py	61	0	100%
types.py	27	1	96%	28
skore/src/skore/_sklearn/_comparison
__init__.py	7	0	100%
feature_importance_accessor.py	39	2	94%	92, 111
metrics_accessor.py	178	3	98%	173, 253, 1215
report.py	106	0	100%
utils.py	54	0	100%
skore/src/skore/_sklearn/_cross_validation
__init__.py	9	0	100%
data_accessor.py	45	3	93%	128, 131, 134
feature_importance_accessor.py	24	0	100%
metrics_accessor.py	182	1	99%	244
report.py	135	1	99%	487
skore/src/skore/_sklearn/_estimator
__init__.py	9	0	100%
data_accessor.py	58	0	100%
feature_importance_accessor.py	144	2	98%	223–224
metrics_accessor.py	356	8	97%	200, 202, 209, 300, 369, 373, 388, 423
report.py	167	2	98%	448–449
skore/src/skore/_sklearn/_plot
__init__.py	3	0	100%
base.py	7	0	100%
style.py	29	0	100%
utils.py	141	7	95%	59, 83–85, 89, 344–345
skore/src/skore/_sklearn/_plot/data
__init__.py	2	0	100%
table_report.py	183	1	99%	682
skore/src/skore/_sklearn/_plot/metrics
__init__.py	6	0	100%
confusion_matrix.py	70	4	94%	91, 99, 121, 229
feature_importance_display.py	67	21	68%	92, 115–116, 118, 136–140, 142–149, 152–154, 156
metrics_summary_display.py	9	0	100%
precision_recall_curve.py	278	5	98%	459, 559, 563, 623, 743
prediction_error.py	225	5	97%	181, 188, 424, 507, 687
roc_curve.py	290	8	97%	389, 512, 517, 618, 623, 627, 696, 818
skore/src/skore/_sklearn/train_test_split
__init__.py	0	0	100%
train_test_split.py	58	0	100%
skore/src/skore/_sklearn/train_test_split/warning
__init__.py	8	0	100%
high_class_imbalance_too_few_examples_warning.py	19	1	94%	83
high_class_imbalance_warning.py	20	0	100%
random_state_unset_warning.py	10	0	100%
shuffle_true_warning.py	9	0	100%
stratify_is_set_warning.py	10	0	100%
time_based_column_warning.py	21	0	100%
train_test_split_warning.py	3	0	100%
skore/src/skore/_utils
__init__.py	6	2	66%	8, 13
_accessor.py	90	3	96%	34, 146, 190
_environment.py	27	0	100%
_fixes.py	8	0	100%
_index.py	5	0	100%
_logger.py	22	4	81%	15–17, 19
_measure_time.py	10	0	100%
_parallel.py	38	3	92%	23, 33, 124
_patch.py	13	5	61%	21, 23–24, 35, 37
_progress_bar.py	46	0	100%
_repr_html.py	8	0	100%
_show_versions.py	38	0	100%
_testing.py	55	0	100%
skore/src/skore/project
__init__.py	2	0	100%
project.py	48	0	100%
summary.py	74	0	100%
widget.py	165	6	96%	436, 439–441, 525–526
TOTAL	3986	116	97%

Tests	Skipped	Failures	Errors	Time
1060	5 💤	0 ❌	0 🔥	4m 24s ⏱️

github-actions · 2025-08-04T16:46:21Z

Documentation preview @ 19504ba

skore/tests/unit/displays/table_report/test_common.py

skore/tests/unit/displays/table_report/test_estimator.py

skore/tests/unit/sklearn/test_find_estimators.py

glemaitre

It looks really. We still need the following:

Similarly to skore/tests/unit/reports/estimator/data/test_accessor.py, we need to create skore/tests/unit/reports/cross-validation/data/test_accessor.py with the same type of tests.
We miss the documentation in the rst file cross_validation_report.rst (we have the same issue for the estimator_report.rst and I'll make a quick PR to solve this problem).
In terms of documentation, we should scan the user guide to see where the information make sense to be added and we should use it somewhere in the example. I recall that it was complex when using the EstimatorReport because our example used the CrossValidationReport so it should be quite easy to find the example where the data analysis will fit.
As commented I'll make a couple of PR that will isolate some fix or refactoring that are not related to the PR itself to make the history cleaner when it comes to the future us using git blame.

skore/tests/unit/displays/table_report/test_common.py

skore/src/skore/__init__.py

skore/src/skore/_sklearn/_cross_validation/report.py

skore/tests/unit/displays/table_report/test_cross_validation.py

glemaitre · 2025-08-30T07:54:06Z

skore/tests/unit/displays/table_report/test_cross_validation.py

+        # pd.Series(np.ones(100)),
+        # pd.DataFrame(np.ones((100, 1)), columns=["Target"]),


I think that they are legit case and we need to test them as well. If there is a bug (I suspect that there is one because I stumbled in #1990), then we need to make a PR to fix it first. So here, I think that we should pass a Series that does not have a name as it is now, and another where we fix the name.

Actually it may have been another error. I fixed it, but also did a ticket on skrub side to be sure: skrub-data/skrub#1593.

skore/tests/unit/displays/table_report/test_cross_validation.py

This PR simplifies #1965 and allow to get a clean history for the `find_estimators` functionality. For the sake of completness, we remove this utility after dropping the feature discussed in #1811 because it was not obvious how to add every components of a pipeline on the parallel coordinate plot.

Co-authored-by: Guillaume Lemaitre <[email protected]>

github-actions · 2025-09-09T15:37:25Z

Caution

Some commits in the pull request are not signed, or GitHub is not able to verify the signature.
Please sign all your commits; you can find more information here.
Please note that when you activate commit signing, you'll need to retroactively sign your previous commits.

glemaitre

LGTM. I will check the documentation rendering before to merge but otherwise, it looks good.

github-actions bot assigned MarieSacksick Aug 4, 2025

MarieSacksick changed the base branch from main to fix_clustering August 6, 2025 11:43

MarieSacksick marked this pull request as ready for review August 6, 2025 12:00

Base automatically changed from fix_clustering to main August 7, 2025 11:04

thomass-dev requested a review from glemaitre August 7, 2025 20:07

glemaitre reviewed Aug 8, 2025

View reviewed changes

skore/tests/unit/displays/table_report/test_common.py Outdated Show resolved Hide resolved

glemaitre reviewed Aug 8, 2025

View reviewed changes

skore/tests/unit/displays/table_report/test_estimator.py Outdated Show resolved Hide resolved

glemaitre reviewed Aug 8, 2025

View reviewed changes

skore/tests/unit/sklearn/test_find_estimators.py Outdated Show resolved Hide resolved

auguste-probabl requested a review from glemaitre August 13, 2025 13:55

glemaitre reviewed Aug 30, 2025

View reviewed changes

glemaitre mentioned this pull request Aug 30, 2025

chore(skore): Remove find_estimators utility #1994

Merged

MarieSacksick marked this pull request as draft September 5, 2025 16:19

MarieSacksick and others added 15 commits September 8, 2025 16:47

start cross-val data accessor

546c92a

add tests and adapt code

860fe58

update tabular learner to pipeline, new in skrub 0.6

b784c7f

fix doctest

d336a23

update tabular learner to pipeline, new in skrub 0.6

0279610

fix doctest

30b5b87

add tests

0566330

split tests common and estimator for display

ab07bcf

shorten some tests

18b4ed9

add test missing y

ae03d63

fix repr

73af646

test repr of data accessor instead of display

ca2d6e2

factorize fixtures

f2945a6

remove find_estimators

1fb6880

vendor tabular_pipeline for skrub compatibility

cb9f729

auguste-probabl and others added 6 commits September 8, 2025 16:55

Update skore/tests/unit/displays/table_report/test_cross_validation.py

16419ce

Co-authored-by: Guillaume Lemaitre <[email protected]>

parametrize tests

040351d

make test name more explicit

0ad5d87

commonize html repr

29ae880

add data accessor to rst file for cv

960adc4

linting

ece0561

MarieSacksick force-pushed the data_accessor_cv branch from cecd1e2 to ece0561 Compare September 8, 2025 14:56

MarieSacksick added 4 commits September 8, 2025 17:28

Adapt tests

c7f848b

enhance tests

b3c529b

complete data acessor docs

e0eaec6

add data to accessor in docs

a0fb0c5

MarieSacksick mentioned this pull request Sep 9, 2025

docs: Add data to accessor in EstimatorReport #2022

Closed

adapt example to use the new data accessor in cv

bfb65f7

MarieSacksick marked this pull request as ready for review September 9, 2025 09:48

remove comment

19504ba

glemaitre approved these changes Sep 9, 2025

View reviewed changes

glemaitre merged commit 84fc6d3 into main Sep 9, 2025
36 checks passed

glemaitre deleted the data_accessor_cv branch September 9, 2025 15:51

		# pd.Series(np.ones(100)),
		# pd.DataFrame(np.ones((100, 1)), columns=["Target"]),

feat: Add the data accessor to CrossValidationReport #1965

feat: Add the data accessor to CrossValidationReport #1965

Uh oh!

Conversation

MarieSacksick commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre left a comment • edited by MarieSacksick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

auguste-probabl Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

MarieSacksick Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2025

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MarieSacksick commented Aug 4, 2025 •

edited

Loading

github-actions bot commented Aug 4, 2025 •

edited

Loading

github-actions bot commented Aug 4, 2025 •

edited

Loading

glemaitre left a comment •

edited by MarieSacksick

Loading