FIX handle aliases in displays when used as default and provided by user #30023

JosephBARBIERDARNAL · 2024-10-07T12:27:48Z

Reference Issue

What does this implement/fix?

Bug summary

When passing additional keyword arguments to the random classifier's line via the chance_level_kw argument, some arguments raise an error even though they are valid matplotlib.pyplot.plot() arguments. The error occurs with the c and ls arguments.

The reason is that in scikit-learn/sklearn/metrics/_plot/roc_curve.py, the following code exists:

chance_level_line_kw = {
    "label": "Chance level (AUC = 0.5)",
    "color": "k",
    "linestyle": "--",
}

if chance_level_kw is not None:
    chance_level_line_kw.update(**chance_level_kw)

Matplotlib raises an error when both color and c, or linestyle and ls are specified (this happens with other arguments too, but these are not relevant here since scikit-learn does not set values for them).

Solution TLDR

If the user supplies ls or c, linestyle and color are overwritten with user inputs, and ls and c are removed from chance_level_kw.

I've added a test for these specific cases.

Any other comments?

At the moment, it only fixes the RocCurveDisplay class. I need to investigate what other classes have the same reproducible bug.

github-actions · 2024-10-07T12:29:28Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: f00f5de. Link to the linter CI: here}

JosephBARBIERDARNAL · 2024-10-07T12:30:54Z

@glemaitre here the fix for #30015 . I'll wait for review before continuing.

glemaitre · 2024-10-07T12:38:24Z

Thanks. We will need an entry in the changelog for sure. This should solve the CI. You can add it in doc/whats_new/v1.6.rst file.

JosephBARBIERDARNAL · 2024-10-07T14:51:59Z

I added the fix for PredictionErrorDisplay.

Note: In metrics/_plot/tests/test_predict_error_display.py, there was a duplicate:

assert display.line_.get_color() == "black"
assert_allclose(display.scatter_.get_edgecolor(), [[1.0, 0.0, 0.0, 0.8]])

This is supposed to test the default parameters, but since display.plot(**extra_params) is used, the default parameters are not actually being tested.

glemaitre · 2024-10-07T15:55:58Z

I add a quick look at the different displays and I think we will have issue in the following:

CalibrationDisplay
PartialDependenceDisplay
ConfusionMatrixDisplay
PrecisionRecallDisplay
PredictionErrorDisplay

In general, it boils down to the same issue due to the aliases:

color or c
linewidth or lw
linestyle or ls

So I'm thinking that we can make a utility tool in sklearn/utils/_plotting.py module.
I think that this utility could be thought with the following signature:

def validate_style_kwargs(default_style_kwargs, user_style_kwargs):
    ...

The job will be to check that the user does not pass two aliases (e.g. lw + linewidth) such that we can safely delete an option afterwards. Once we are sure, we return a dictionary that use only one of the alias. Let's avoid to call del on the dict but instead create the dictionary.

Then in each function, we only have to call the utility whenever we want to pass it to the plotting function from matplotlib.

In terms of test, we therefore only need to check our small utility function.

JosephBARBIERDARNAL · 2024-10-07T17:32:29Z

So I'm thinking that we can make a utility tool in sklearn/utils/_plotting.py module.
I think that this utility could be thought with the following signature:

I agree, I'll do that

glemaitre

It looks good. Could you also check the remaining classes for the different kwargs.

sklearn/utils/_plotting.py

sklearn/metrics/_plot/regression.py

sklearn/metrics/_plot/roc_curve.py

glemaitre · 2024-10-07T20:09:03Z

sklearn/metrics/_plot/tests/test_roc_curve_display.py

+    [
+        None,
+        {"linewidth": 1, "color": "red", "linestyle": "-", "label": "DummyEstimator"},
+        {"linewidth": 1, "c": "red", "ls": "-", "label": "DummyEstimator"},


maybe lw here instead of linewidth

sklearn/utils/tests/test_plotting.py

JosephBARBIERDARNAL · 2024-10-08T07:48:40Z

It looks good. Could you also check the remaining classes for the different kwargs.

Yes

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

JosephBARBIERDARNAL · 2024-10-09T13:14:41Z

RocCurveDisplay
CalibrationDisplay
ConfusionMatrixDisplay
PrecisionRecallDisplay
PredictionErrorDisplay
PartialDependenceDisplay: I've made a few changes but I can't get a few tests (assert line.get_color() == expected_colors[0]) to pass. My changes probably handle kw priorities incorrectly. Some help would be appreciated.

JosephBARBIERDARNAL · 2024-10-09T13:34:55Z

Also, I'm not sure in which section of the changelog I should add the changes? Most of them are in sklearn.metrics but not all:

- |Fix| the classes :class:`metrics.ConfusionMatrixDisplay`, :class:`metrics.ROCCurveDisplay`,
  :class:`calibration.CalibrationDisplay`, :class:`metrics.PrecisionRecallDisplay`,
  :class:`metrics.PredictionErrorDisplay` and :class:`inspection.PartialDependenceDisplay`
  now properly handle Matplotlib aliases for style parameters (e.g., `c` and `color`, `ls`
  and `linestyle`, etc).
  :pr:`30023` by :user:`Joseph Barbier <JosephBARBIERDARNAL>` and :user:`Guillaume Lemaitre <glemaitre>`.

glemaitre · 2024-10-09T15:06:11Z

On the top of the changelog with have a section called "Changes impacting many modules". I think we can add the entry in this section because as you mentioned, we are touching classes from several modules.

glemaitre · 2024-10-09T15:09:04Z

I'll do a review. I might push a couple of fixes just to make the CI happy first.

JosephBARBIERDARNAL · 2024-10-09T17:39:32Z

I've just made the changes @glemaitre

What is missing for the CI to pass? I use the pre-commit hook on my side but it still seems to have linting problems.

sklearn/utils/tests/test_plotting.py

sklearn/inspection/_plot/tests/test_plot_partial_dependence.py

sklearn/metrics/_plot/tests/test_roc_curve_display.py

glemaitre

Once the last change is done, everything looks good to me.

Thanks @JosephBARBIERDARNAL.

We will need to get a second review to get this one in. Let me ping maybe @lucyleeow or @Charlie-XIAO

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

JosephBARBIERDARNAL · 2024-10-11T14:33:54Z

Thanks @JosephBARBIERDARNAL.

Perfect, thanks for the prompt reviews.

Charlie-XIAO

Thanks @JosephBARBIERDARNAL this PR looks mostly good to me! Just a few questions/comments:

sklearn/utils/tests/test_plotting.py

Charlie-XIAO · 2024-10-11T14:53:21Z

sklearn/utils/_plotting.py

+        Matplotlib style kwargs.
+    """
+
+    invalid_to_valid_kw = {


May I know where this mapping came from? I searched in the matplotlib source code where they used their define_aliases decorator, and there seems to be many more that we missed here (though maybe not all of them are related here).

https://github.com/search?q=repo%3Amatplotlib%2Fmatplotlib+define_aliases&type=code

Yes I made it myself, more or less arbitrarily. But since matplotlib has a clearly defined list of aliases, maybe let's just put the entire list instead?

Yes indeed. We could even import it because we expect to have matplotlib installed.

I did a quick check, and I'm not sure how to import it in practice?

Yep, I could not find a non-private way of getting them. So I would define the list such that in account for the keys/values of the matplotlib artist that we are using.

FYI, our function is close to this function (https://github.com/matplotlib/matplotlib/blob/v3.9.2/lib/matplotlib/cbook.py#L1795-L1848) when it comes to matplotlib itself. No need to try using it thought.

So I've added all the aliases, except for those used for 3D plotting

LGTM. Thanks @JosephBARBIERDARNAL

Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>

glemaitre

@Charlie-XIAO do you want to have another look?

glemaitre · 2024-10-16T14:44:09Z

sklearn/utils/_plotting.py

+        Matplotlib style kwargs.
+    """
+
+    invalid_to_valid_kw = {


LGTM. Thanks @JosephBARBIERDARNAL

Charlie-XIAO

LGTM, thanks @JosephBARBIERDARNAL! There's just a small typo:

doc/whats_new/v1.6.rst

Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>

glemaitre · 2024-10-17T19:23:56Z

We just change the way that we add entry into the changelog. Let me make the change and push it.

…L/30023

glemaitre

I added the fragment in the changelog. Enabling the auto-merge then.

Thanks @JosephBARBIERDARNAL. Nice contribution.

JosephBARBIERDARNAL · 2024-10-17T19:54:58Z

Thanks! This is my very first merged PR, and it's been a really good experience.

JosephBARBIERDARNAL added 2 commits October 7, 2024 14:17

fix invalid chance_level_line_kw in roccurvedisplay

e98cf0b

fix invalid chance_level_line_kw in roccurvedisplay

829c67f

github-actions bot added the module:metrics label Oct 7, 2024

Merge branch 'main' into fix-chance_level_kw

c7bda19

fix style kw for PredictionErrorDisplay

a3e3984

create validate style params utils for display class

bbce88e

glemaitre self-requested a review October 7, 2024 19:50

glemaitre reviewed Oct 7, 2024

View reviewed changes

JosephBARBIERDARNAL and others added 6 commits October 8, 2024 09:51

ensure user does not provide style aliases

672bbbc

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

pass style params by position

546ca5a

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

shorter test case for plotting

99f1786

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

shortest test case for plotting

277d376

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

more explicit description on invalid test case

646dde2

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Merge branch 'main' into fix-chance_level_kw

8317063

glemaitre changed the title ~~fix invalid inputs for chance_level_line_kw in display classes~~ FIX handle aliases in displays when used as default and provided by user Oct 8, 2024

better aliases handling in displays

9e509e7

JosephBARBIERDARNAL and others added 2 commits October 9, 2024 15:15

Merge branch 'main' into fix-chance_level_kw

41f55b5

remove commented code

7d4edcd

glemaitre self-requested a review October 9, 2024 15:09

better aliases handling in displays

bc9ee76

glemaitre reviewed Oct 9, 2024

View reviewed changes

sklearn/utils/tests/test_plotting.py Outdated Show resolved Hide resolved

glemaitre reviewed Oct 9, 2024

View reviewed changes

sklearn/utils/tests/test_plotting.py Outdated Show resolved Hide resolved

glemaitre added 2 commits October 9, 2024 20:40

Update sklearn/utils/tests/test_plotting.py

cd938b9

remove duplicated test

5dc226f

glemaitre reviewed Oct 9, 2024

View reviewed changes

sklearn/inspection/_plot/tests/test_plot_partial_dependence.py Show resolved Hide resolved

JosephBARBIERDARNAL and others added 3 commits October 10, 2024 09:48

test for aliases in PDD

631be6a

Merge branch 'main' into fix-chance_level_kw

086954f

add test for plotting util

84109fb

glemaitre reviewed Oct 11, 2024

View reviewed changes

sklearn/metrics/_plot/tests/test_roc_curve_display.py Outdated Show resolved Hide resolved

glemaitre added this to the 1.6 milestone Oct 11, 2024

glemaitre approved these changes Oct 11, 2024

View reviewed changes

Update sklearn/metrics/_plot/tests/test_roc_curve_display.py

f9e9207

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Charlie-XIAO reviewed Oct 11, 2024

View reviewed changes

Update sklearn/utils/tests/test_plotting.py

b8b6723

Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>

glemaitre self-requested a review October 14, 2024 21:16

better matplotlib aliases list

a9d1b0b

glemaitre approved these changes Oct 16, 2024

View reviewed changes

sklearn/utils/_plotting.py

Matplotlib style kwargs.

"""

invalid_to_valid_kw = {

Copy link

Member

glemaitre Oct 16, 2024

Choose a reason for hiding this comment

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

LGTM. Thanks @JosephBARBIERDARNAL

Charlie-XIAO approved these changes Oct 16, 2024

View reviewed changes

doc/whats_new/v1.6.rst Outdated Show resolved Hide resolved

Update doc/whats_new/v1.6.rst

7f7aca9

Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>

glemaitre self-requested a review October 17, 2024 19:23

Merge remote-tracking branch 'origin/main' into pr/JosephBARBIERDARNA…

f00f5de

…L/30023

glemaitre approved these changes Oct 17, 2024

View reviewed changes

glemaitre enabled auto-merge (squash) October 17, 2024 19:27

glemaitre merged commit 8b06fa6 into scikit-learn:main Oct 17, 2024
30 checks passed

JosephBARBIERDARNAL mentioned this pull request Nov 3, 2024

ENH add CAP curve #28972

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX handle aliases in displays when used as default and provided by user #30023

FIX handle aliases in displays when used as default and provided by user #30023

JosephBARBIERDARNAL commented Oct 7, 2024 •

edited by glemaitre

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre commented Oct 7, 2024

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre commented Oct 7, 2024 •

edited

Loading

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre left a comment

glemaitre Oct 7, 2024

JosephBARBIERDARNAL commented Oct 8, 2024

JosephBARBIERDARNAL commented Oct 9, 2024 •

edited by glemaitre

Loading

JosephBARBIERDARNAL commented Oct 9, 2024

glemaitre commented Oct 9, 2024

glemaitre commented Oct 9, 2024

JosephBARBIERDARNAL commented Oct 9, 2024 •

edited

Loading

glemaitre left a comment

JosephBARBIERDARNAL commented Oct 11, 2024

Charlie-XIAO left a comment

Charlie-XIAO Oct 11, 2024

JosephBARBIERDARNAL Oct 11, 2024

glemaitre Oct 11, 2024

JosephBARBIERDARNAL Oct 13, 2024

glemaitre Oct 14, 2024

JosephBARBIERDARNAL Oct 15, 2024

glemaitre Oct 16, 2024

glemaitre left a comment

glemaitre Oct 16, 2024

Charlie-XIAO left a comment

glemaitre commented Oct 17, 2024

glemaitre left a comment

JosephBARBIERDARNAL commented Oct 17, 2024

FIX handle aliases in displays when used as default and provided by user #30023

FIX handle aliases in displays when used as default and provided by user #30023

Conversation

JosephBARBIERDARNAL commented Oct 7, 2024 • edited by glemaitre Loading

Reference Issue

What does this implement/fix?

Any other comments?

github-actions bot commented Oct 7, 2024 • edited Loading

✔️ Linting Passed

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre commented Oct 7, 2024

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre commented Oct 7, 2024 • edited Loading

JosephBARBIERDARNAL commented Oct 7, 2024

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephBARBIERDARNAL commented Oct 8, 2024

JosephBARBIERDARNAL commented Oct 9, 2024 • edited by glemaitre Loading

JosephBARBIERDARNAL commented Oct 9, 2024

glemaitre commented Oct 9, 2024

glemaitre commented Oct 9, 2024

JosephBARBIERDARNAL commented Oct 9, 2024 • edited Loading

glemaitre left a comment

Choose a reason for hiding this comment

JosephBARBIERDARNAL commented Oct 11, 2024

Charlie-XIAO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charlie-XIAO left a comment

Choose a reason for hiding this comment

glemaitre commented Oct 17, 2024

glemaitre left a comment

Choose a reason for hiding this comment

JosephBARBIERDARNAL commented Oct 17, 2024

JosephBARBIERDARNAL commented Oct 7, 2024 •

edited by glemaitre

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

glemaitre commented Oct 7, 2024 •

edited

Loading

JosephBARBIERDARNAL commented Oct 9, 2024 •

edited by glemaitre

Loading

JosephBARBIERDARNAL commented Oct 9, 2024 •

edited

Loading