MNT refactor _get_response_values #21538

glemaitre · 2021-11-03T10:48:25Z

Partially address #18212
This is a simplification towards merging #20999

glemaitre · 2021-11-03T13:30:13Z

sklearn/metrics/_plot/tests/test_plot_precision_recall.py

@@ -26,72 +24,6 @@
 )


-def test_errors(pyplot):


Those tests are moved in the common test in test_plot_curve_common.py

glemaitre · 2021-11-03T13:30:30Z

sklearn/tests/test_calibration.py

@@ -634,42 +634,6 @@ def iris_data_binary(iris_data):
    return X[y < 2], y[y < 2]


-def test_calibration_display_validation(pyplot, iris_data, iris_data_binary):


Those tests are moved in the common test in test_plot_curve_common.py

glemaitre · 2021-11-03T13:30:52Z

sklearn/tests/test_calibration.py

@@ -189,7 +189,7 @@ def test_parallel_execution(data, method, ensemble):
    X, y = data
    X_train, X_test, y_train, y_test = train_test_split(X, y, random_state=42)

-    base_estimator = LinearSVC(random_state=42)
+    base_estimator = make_pipeline(StandardScaler(), LinearSVC(random_state=42))


Remove a bunch of ConvergenceWarning for free

glemaitre · 2021-11-03T13:31:41Z

sklearn/metrics/_plot/tests/test_precision_recall_display.py

@@ -21,48 +20,6 @@
 )


-def test_precision_recall_display_validation(pyplot):


Those tests are moved in the common test in plot_common_curve_display.py

glemaitre · 2021-11-03T14:14:13Z

Maybe this would be easier to review @ogrisel @thomasjpfan

thomasjpfan

Overall, I like the refactor.

sklearn/metrics/_plot/base.py

sklearn/metrics/_plot/det_curve.py

sklearn/metrics/_plot/precision_recall_curve.py

sklearn/utils/validation.py

sklearn/utils/_mocking.py

sklearn/utils/__init__.py

sklearn/utils/validation.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan

Thanks for the follow up!

Do we need this check here, since we do the check in det_curve?

scikit-learn/sklearn/metrics/_ranking.py

Lines 310 to 314 in 845b1fa

    
           if len(np.unique(y_true)) != 2: 
        
               raise ValueError( 
        
                   "Only one class present in y_true. Detection error " 
        
                   "tradeoff curve is not defined in that case." 
        
               )

sklearn/metrics/_plot/precision_recall_curve.py

…nto is/18212_again_again

lesteve

First pass of comments

lesteve · 2022-04-06T10:15:40Z

sklearn/utils/tests/test_utils.py

+        response_method="predict",
+        target_type=target_type,
+    )
+    assert_allclose(y_pred, regressor.predict(X))


This one could be checking strict equality right?

lesteve · 2022-04-06T13:45:13Z

sklearn/utils/validation.py

+
+    Raises
+    ------
+    ValueError


Suggested change

ValueError

AttributeError

lesteve · 2022-04-06T14:38:17Z

sklearn/utils/__init__.py

+                    y_pred = y_pred[:, col_idx]
+                else:
+                    err_msg = (
+                        f"Got predict_proba of shape {y_pred.shape}, but need "


I find the error message not super explicit, I would find it easier to understand if it was saying the classifier was fitted with only one class (rather that complaining about the shape of the predict_proba). Maybe there are some edge cases I haven't thought of though.

lesteve · 2022-04-06T14:46:20Z

sklearn/utils/__init__.py

+                f"one of {classes}"
+            )
+        elif pos_label is None and target_type == "binary":
+            pos_label = pos_label if pos_label is not None else classes[-1]


Although -1 and 1 are equivalent I would use 1 since classes[1] is used elsewhere e.g. in _get_response_values docstring:

Suggested change

pos_label = pos_label if pos_label is not None else classes[-1]

pos_label = pos_label if pos_label is not None else classes[1]

lesteve · 2022-04-06T14:54:39Z

sklearn/ensemble/_stacking.py

+        method = (
+            ["predict_proba", "decision_function", "predict"]
+            if method == "auto"
+            else method
+        )


A bit lighter to parse IMO (and also what is used in other places in this PR):

Suggested change

method = (

["predict_proba", "decision_function", "predict"]

if method == "auto"

else method

)

if method == "auto":

method = ["predict_proba", "decision_function", "predict"]

lesteve · 2022-04-06T14:57:13Z

sklearn/metrics/_plot/base.py

+            "'fit' with appropriate arguments before intending to use it to plotting "
+            "functionalities."


Maybe a bit lighter to read (better suggestion welcome):

Suggested change

"'fit' with appropriate arguments before intending to use it to plotting "

"functionalities."

"'fit' with appropriate arguments before using it for plotting "

"functionalities."

lesteve · 2022-04-06T15:35:34Z

The CI is red at the moment, maybe I got something wrong in my merge ...

thomasjpfan

Comment that still needs to be addressed, with the same concern:

~~The metric checks if the input is binary. If the *Display object checks too, then there is double validation and np.unqiue is called twice.~~

I guess this is okay.

thomasjpfan · 2022-04-06T18:49:29Z

sklearn/metrics/_plot/roc_curve.py

@@ -330,6 +342,12 @@ def from_predictions(
        """
        check_matplotlib_support(f"{cls.__name__}.from_predictions")

+        target_type = type_of_target(y_true)
+        if target_type != "binary":


I do not think we need this check. roc_curve calls _binary_clf_curve which ends up doing the binary check itself:

scikit-learn/sklearn/metrics/_ranking.py

Lines 736 to 738 in 22ca942

y_type = type_of_target(y_true, input_name="y_true")

if not (y_type == "binary" or (y_type == "multiclass" and pos_label is not None)):

raise ValueError("{0} format is not supported".format(y_type))

jeremiedbb · 2022-04-19T13:03:53Z

Let's move that to 1.2

glemaitre · 2023-03-24T18:03:39Z

Closing this one since we merged #23073

MNT refactor _get_response_values [ci skip]

51a3bcb

github-actions bot added module:metrics module:utils labels Nov 3, 2021

iter

8675def

glemaitre added the No Changelog Needed label Nov 3, 2021

glemaitre commented Nov 3, 2021

View reviewed changes

iter

64dfc86

thomasjpfan reviewed Nov 28, 2021

View reviewed changes

thomasjpfan reviewed Nov 29, 2021

View reviewed changes

sklearn/utils/validation.py Show resolved Hide resolved

github-actions bot added the cython label Nov 29, 2021

glemaitre added 2 commits November 29, 2021 16:17

Merge remote-tracking branch 'origin/main' into is/18212_again_again

5a4f33f

reuse type_of_target

a7e46da

glemaitre force-pushed the is/18212_again_again branch from abea58f to a7e46da Compare November 29, 2021 15:18

glemaitre and others added 3 commits November 29, 2021 16:33

iter

a69176d

make response_method to not be None

8214f8b

Update sklearn/utils/_mocking.py

d7e4912

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

8000
glemaitre removed the cython label Nov 29, 2021

glemaitre added 5 commits November 29, 2021 17:08

iter

75bd0e1

iter

04b54c9

iter

9e834b4

iter

3487701

iter

2920e32

thomasjpfan reviewed Dec 15, 2021

View reviewed changes

glemaitre added this to the 1.1 milestone Jan 26, 2022

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

8863fad

…nto is/18212_again_again

lesteve reviewed Apr 6, 2022

View reviewed changes

thomasjpfan reviewed Apr 6, 2022

View reviewed changes

glemaitre mentioned this pull request Apr 7, 2022

MAINT introduce _get_response_values and _check_response_methods #23073

Merged

jeremiedbb modified the milestones: 1.1, 1.2 Apr 19, 2022

glemaitre modified the milestones: 1.2, 1.3 Nov 16, 2022

glemaitre closed this Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT refactor _get_response_values #21538

MNT refactor _get_response_values #21538

		@@ -634,42 +634,6 @@ def iris_data_binary(iris_data):
		return X[y < 2], y[y < 2]


		def test_calibration_display_validation(pyplot, iris_data, iris_data_binary):

		@@ -21,48 +20,6 @@
		)


		def test_precision_recall_display_validation(pyplot):

	if len(np.unique(y_true)) != 2:
	raise ValueError(
	"Only one class present in y_true. Detection error "
	"tradeoff curve is not defined in that case."
	)

	pos_label = pos_label if pos_label is not None else classes[-1]
	pos_label = pos_label if pos_label is not None else classes[1]

		"'fit' with appropriate arguments before intending to use it to plotting "
		"functionalities."

	y_type = type_of_target(y_true, input_name="y_true")
	if not (y_type == "binary" or (y_type == "multiclass" and pos_label is not None)):
	raise ValueError("{0} format is not supported".format(y_type))

MNT refactor _get_response_values #21538

MNT refactor _get_response_values #21538

Conversation

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

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