Skip to content

docs: Add SKD003 inconsistent performance pitfall example - #3145

Open
moujanrastgoo wants to merge 5 commits into
probabl-ai:mainfrom
moujanrastgoo:docs/add-skd003-pitfall-example
Open

docs: Add SKD003 inconsistent performance pitfall example#3145
moujanrastgoo wants to merge 5 commits into
probabl-ai:mainfrom
moujanrastgoo:docs/add-skd003-pitfall-example

Conversation

@moujanrastgoo

@moujanrastgoo moujanrastgoo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Change description

Related to #2622 and #3134

  • Adds new sphinx-gallery subsection Pitfalls and solutions
  • Registers subsection in sphinx/conf.py
  • Adds SKD003 check exmaple and links from automated checks user guide

Contribution checklist

  • Unit tests were added or updated (if necessary)
  • Documentation was added or updated (if necessary)
  • The code passes our style conventions (you can check this by running pre-commit on your code
    with pre-commit run --all-files)
  • All the tests pass (please test locally before pushing)
  • The documentation builds and renders properly (if it does, our bot will add a comment linking
    to a preview of the documentation to review it visually)
  • All the commits in the PR are signed (more information
    here)
  • The pull request title respects the Conventional Commits convention (more information
    here)

AI usage disclosure

AI tools were involved for:

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

The use of AI regarding documentation was mostly for formatting the doc files and checking for errors and debugging, not for creating the examples themselves.

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Documentation preview @ 2cc7c40

Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated

@GaetandeCast GaetandeCast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my general thoughts on this check and a few other minor remarks

Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated
Comment thread examples/pitfalls_and_solutions/plot_skd003_inconsistent_performance.py Outdated

@GaetandeCast GaetandeCast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some remarks

Comment on lines +32 to +34
problem is understood. We focus on SKD003 throughout and pass
``ignore=["SKD008"]`` so Covertype's constant one-hot soil columns do not
drown the summary in correlated-feature noise.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not exploiting anything specific from this dataset (I think we don't but correct me if I'm wrong), let's just switch to a dataset that has no constant columns to avoid that concern altogether.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure i will look into that!

outlier_report.metrics.summarize(data_source="test").frame()

# %%
outlier_report.metrics.precision_recall().plot()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think people are more used to roc plots so I'd rather use one here. It is easy to show that something is wrong on a roc plot since the plot will follow the diagonal "chance level" line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok sure!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Can you also catch the output:

_ = outlier_report.metrics.roc().plot()

report.metrics.summarize(data_source="test").frame(aggregate=None)

# %%
# ``SKD003`` should flag split #0.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ``SKD003`` should flag split #0.
# ``SKD003`` correctly flags split #0.

Here and for other occurences.

Comment on lines +196 to +203
X_arr, y_arr = make_classification(
n_samples=n_time,
n_features=6,
n_informative=2,
n_redundant=0,
weights=[0.5, 0.5],
random_state=0,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather we add a fake datetime column to the same dataset used in the previous sections for consistency, just like how we created fake groups, and then change the distribution in the last temporal group.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@glemaitre
glemaitre self-requested a review July 30, 2026 08:01

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a couple of comments.

===============================================

:ref:`SKD003 <skd003-inconsistent-performance>` flags folds whose test metrics
diverge sharply from the median on a :class:`~skore.CrossValidationReport`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should focus on the stats words instead of the Python object.

Suggested change
diverge sharply from the median on a :class:`~skore.CrossValidationReport`.
diverge sharply from the median during a cross-validation evaluation.

- a much easier or harder group in one test fold under
:class:`~sklearn.model_selection.GroupKFold`,
- temporal drift under :class:`~sklearn.model_selection.TimeSeriesSplit`
(e.g. an easy class that becomes rarer),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the example:

Suggested change
(e.g. an easy class that becomes rarer),
(e.g. fraudsters finding workarounds to circumvent controls),

:ref:`SKD003 <skd003-inconsistent-performance>` flags folds whose test metrics
diverge sharply from the median on a :class:`~skore.CrossValidationReport`.
With a proper splitter this is often a diagnostic check: the data have structure
(groups, time, or a corrupted batch) that shuffled CV would hide.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use whenever possible "cross-validation" instead of "CV"

Comment on lines +42 to +43
# subsample. SKD003 needs CV, so we use ``splitter=5`` or an explicit five-fold
# splitter below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To early to speak about the cross-validation. Let's only discuss it when we call skore.evaluate.

Comment on lines +78 to +81
# Randomly reassign labels on the first fifth of rows (mislabelled batch,
# logging bug, or merge mix-up). ``splitter=5`` is unshuffled
# :class:`~sklearn.model_selection.KFold`, so that block stays in fold #0 and
# SKD003 can flag it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Randomly reassign labels on the first fifth of rows (mislabelled batch,
# logging bug, or merge mix-up). ``splitter=5`` is unshuffled
# :class:`~sklearn.model_selection.KFold`, so that block stays in fold #0 and
# SKD003 can flag it.
# Below, we will use an unshuffled 5-fold cross-validation. Here, we permute
# labels on the first fold to break any association between `X` and `y`. We expect
# the score of this fold to be low due to this corruption.

size=int(bad_batch.sum()),
)

group_splits = list(GroupKFold(n_splits=5).split(X, y_batch, groups=groups))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have to create a list of index here. But because it is an advance use case we need to use a skrub DataOp. We probably need to have some narrative to explain that to the user by:

  1. generating the data
  2. explaining the creation of the skrub data op and for which reason
  3. make the evaluation and explain the problem at hand

The implementation of the skrub DataOp here will look something like:

# %%
# Bad group in the test fold
# ==========================
#
# With group ids (site, patient, batch), a grouped splitter keeps each group on
# one side of every split. A much harder or easier group in one test fold
# triggers SKD003: expected, the splitter did its job. Shuffled CV would smear
# that group across folds, hide the gap, and overestimate performance.
#
# Below we invent batch ids and corrupt labels for batch ``0`` only. Instead of
# materializing a list of train/test indices, we keep features, target, and
# group ids in a single dataframe: the group ids then follow the rows through
# every split.

n_batches = 10
batch_id = np.minimum(np.arange(len(X)) // (len(X) // n_batches), n_batches - 1)

y_batch = y_clean.copy()
bad_batch = batch_id == 0
y_batch.iloc[bad_batch] = np.random.RandomState(1).choice(
    y_clean.unique(),
    size=int(bad_batch.sum()),
)

df_batch = X.assign(batch_id=batch_id, cover_type=y_batch)

# %%
# A skrub :class:`~skrub.DataOp` declares the split on the data itself:
# :meth:`~skrub.DataOp.skb.mark_as_X` takes the splitter through ``cv`` and the
# group ids through ``split_kwargs``. The resulting learner carries its own
# cross-validation scheme, so :func:`~skore.evaluate` needs no ``splitter``.

import skrub
from sklearn.model_selection import GroupKFold

data = skrub.var("data", df_batch)
groups = data["batch_id"]
X_op = data.drop(columns=["batch_id", "cover_type"]).skb.mark_as_X(
    cv=GroupKFold(n_splits=5),
    split_kwargs={"groups": groups},
)
y_op = data["cover_type"].skb.mark_as_y()
learner = X_op.skb.apply(model, y=y_op).skb.make_learner()

# %%
report_grouped = skore.evaluate(learner, data={"data": df_batch}, n_jobs=4)
report_grouped.metrics.summarize(data_source="test").frame(aggregate=None)

# ===============================================
#
# Under :class:`~sklearn.model_selection.TimeSeriesSplit`, later windows can
# diverge from early training data (e.g. an easy class becomes rarer). Earlier

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the frauds cases.

#
# Build a balanced task with :func:`~sklearn.datasets.make_classification`,
# treat row order as time, and replace labels only in the final test window
# with a rare positive class so that one late fold stands out. No shuffle.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remarks about the imperative tense :)

)
X_time = pd.DataFrame(X_arr, columns=[f"f{i}" for i in range(6)])
y_time = pd.Series(y_arr, name="label")
y_time.iloc[hard_start:] = rng.choice([0, 1], size=n_time - hard_start, p=[0.96, 0.04])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you separate the data generative process from the evaluation.

y_time = pd.Series(y_arr, name="label")
y_time.iloc[hard_start:] = rng.choice([0, 1], size=n_time - hard_start, p=[0.96, 0.04])

time_splits = list(TimeSeriesSplit(n_splits=n_splits).split(X_time))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not have to materialize as a list. Pass directly the splitter:

time_splitter = TimeSeriesSplit(n_splits=n_splits)
skore.evaluate(..., splitter=time_splitter, ...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that since we introduce a skrub DataOp earlier, we could actually create a timestamp as index, and create our own TimeSeriesSplit that is based on the time. Then, in the data generative process, we could make that for instance the last fold that will be the year "2026", drift compared to previous year.

I find that showing the dataframe and the time and potentially making associated plot will be easier to read.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the dataset, I think that the bike sharing (e.g. https://scikit-learn.org/stable/auto_examples/applications/plot_time_series_lagged_features.html) has this kind of drift: it is a 2 year collection data and the second year as more demand than the first one. Therefore, learning the trend on the first year will not help to compensate the trend of the second year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants