ENH Add routing to LogisticRegressionCV #26525

OmarManzoor · 2023-06-07T07:17:04Z

Reference Issues/PRs

Closes #25906
Fixes #8950
Follow up of #24498

What does this implement/fix? Explain your changes.

Adds routing of additional parameters including sample weight to LogisticRegressionCV
The routing is added in the fit and score methods
A test is added for checking that the scores different when sample weight is requested compared to when it is not

Any other comments?

cc: @adrinjalali
Should I also add the tests that I added for scorers and splitters in test_metaestimators_metadata_routing.py? Also should any additional tests be added now that we have two scenarios, one where the config enable_metadata_routing is True and another where it is False?

…re method

adrinjalali · 2023-06-08T13:16:34Z

re:tests: yes, they were quite nice, it'd be nice to have them here.

adrinjalali

This is great, I'd also add a changelog to 1.4.

sklearn/linear_model/_logistic.py

sklearn/linear_model/tests/test_logistic.py

sklearn/tests/test_metadata_routing.py

sklearn/tests/test_metaestimators_metadata_routing.py

OmarManzoor · 2023-06-25T13:12:19Z

This is great, I'd also add a changelog to 1.4.

Should this be a feature?

github-actions · 2023-06-25T13:14:01Z

✔️ Linting Passed

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

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

adrinjalali

Thanks @OmarManzoor this is great!

adrinjalali · 2023-07-07T08:16:11Z

@OmarManzoor there are some merge conflicts which need to be resolved.

@thomasjpfan @glemaitre this is ready for a second review.

adrinjalali · 2023-07-10T10:37:02Z

@OmarManzoor I think the codecov errors are legit and needs testing.

…ogisticRegressionCV

doc/whats_new/v1.4.rst

glemaitre · 2023-07-12T19:16:42Z

sklearn/linear_model/_logistic.py

+        if params and not _routing_enabled():
+            raise ValueError(
+                "params is only supported if enable_metadata_routing=True."
+                " See the User Guide for more information."


Would be useful to get the link to the user guide.

I mean this page: https://scikit-learn.org/stable/metadata_routing.html

I think it will definitely be useful. But if we add it here in one place whereas there are a number of other cases like this (this particular ValueError) that don't contain this link, won't that be a bit inconsistent?

We can open a subsequent PR to improve the consistency.

sklearn/linear_model/tests/test_logistic.py

glemaitre · 2023-07-12T19:20:28Z

sklearn/linear_model/tests/test_logistic.py

+    assert pytest.approx(lr_cv1.scores_[1]) != lr_cv2.scores_[1]
+
+
+def test_lr_cv_scores_without_enabling_metadata_routing():


Same here regarding an informative docstring.

glemaitre · 2023-07-12T19:45:26Z

sklearn/tests/test_metaestimators_metadata_routing.py

+@pytest.mark.parametrize(
+    "cv_scorer",
+    CV_SCORERS,
+)


Suggested change

@pytest.mark.parametrize(

"cv_scorer",

CV_SCORERS,

)

@pytest.mark.parametrize("cv_scorer", CV_SCORERS)

sklearn/tests/test_metaestimators_metadata_routing.py

glemaitre · 2023-07-12T19:46:13Z

sklearn/tests/test_metaestimators_metadata_routing.py

+@pytest.mark.parametrize(
+    "cv_splitter",
+    CV_SPLITTERS,
+)


Suggested change

@pytest.mark.parametrize(

"cv_splitter",

CV_SPLITTERS,

)

@pytest.mark.parametrize("cv_splitter", CV_SPLITTERS)

sklearn/tests/test_metaestimators_metadata_routing.py

glemaitre · 2023-07-12T19:50:36Z

sklearn/tests/test_metadata_routing.py

@@ -109,12 +109,18 @@ def record_metadata(obj, method, record_default=True, **kwargs):
    obj._records[method] = kwargs


-def check_recorded_metadata(obj, method, **kwargs):
+def check_recorded_metadata(obj, method, split_params=tuple(), **kwargs):
    """Check whether the expected metadata is passed to the object's method."""


I think that it will start to be worth documented the parameter here.
For instance what is the meaning of split_params

glemaitre · 2023-07-13T17:35:07Z

sklearn/linear_model/tests/test_logistic.py

+@pytest.mark.usefixtures("enable_slep006")
+def test_lr_cv_scores_differ_when_sample_weight_is_requested():
+    """Test sample_weight is correctly passed to the scorer in
+    LogisticRegressionCV :meth:`fit` by checking the difference


Suggested change

LogisticRegressionCV :meth:`fit` by checking the difference

`LogisticRegressionCV.fit` by checking the difference

glemaitre · 2023-07-13T17:35:15Z

sklearn/linear_model/tests/test_logistic.py

@@ -2065,6 +2076,54 @@ def test_liblinear_not_stuck():
        clf.fit(X_prep, y)


+@pytest.mark.usefixtures("enable_slep006")
+def test_lr_cv_scores_differ_when_sample_weight_is_requested():
+    """Test sample_weight is correctly passed to the scorer in


Suggested change

"""Test sample_weight is correctly passed to the scorer in

"""Test `sample_weight` is correctly passed to the scorer in

glemaitre · 2023-07-13T17:35:26Z

sklearn/linear_model/tests/test_logistic.py

+def test_lr_cv_scores_differ_when_sample_weight_is_requested():
+    """Test sample_weight is correctly passed to the scorer in
+    LogisticRegressionCV :meth:`fit` by checking the difference
+    in scores with the case when sample_weight is not requested.


Suggested change

in scores with the case when sample_weight is not requested.

in scores with the case when `sample_weight` is not requested.

…mple_weight_is_requested

glemaitre

LGTM. Thanks @OmarManzoor

Co-authored-by: Omar Salman <omar.salman@arbisoft> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

lorentzenchr · 2023-11-15T13:25:58Z

sklearn/linear_model/_logistic.py

+    *,
+    pos_class,
+    Cs,
+    scoring,
+    fit_intercept,
+    max_iter,
+    tol,
+    class_weight,
+    verbose,
+    solver,
+    penalty,
+    dual,
+    intercept_scaling,
+    multi_class,
+    random_state,
+    max_squared_sum,
+    sample_weight,
+    l1_ratio,
+    score_params,


With the removal of the default parameters, the docstring would need their removal too.

Co-authored-by: Omar Salman <omar.salman@arbisoft> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Add routing to LogisticRegressionCV

7952cce

github-actions bot added the module:linear_model label Jun 7, 2023

Add a test with enable_metadata_routing=False and fix an issue in sco…

66ad513

…re method

Add metaestimator tests and fix passing routed params in score method

7e8b824

OmarManzoor marked this pull request as ready for review June 13, 2023 15:23

adrinjalali reviewed Jun 23, 2023

View reviewed changes

PR suggestions

d7e50a6

Omar Salman added 5 commits June 26, 2023 11:22

Merge branch 'main' into logistic_cv_routing

3844706

Add changelog entry

0866c42

Add user and pr information

43f971b

Changelog adjustment

db63769

Remove repr method from ConsumingScorer

a9b984f

adrinjalali mentioned this pull request Jul 7, 2023

SLEP006 - Metadata Routing task list #22893

Open

70 tasks

adrinjalali approved these changes Jul 7, 2023

View reviewed changes

adrinjalali and others added 2 commits July 7, 2023 10:18

Merge branch 'main' into logistic_cv_routing

637c18e

Adjust changelog

314bc83

Add tests for error when passing params when routing not enabled in L…

9a8ef4e

…ogisticRegressionCV

glemaitre self-requested a review July 12, 2023 14:50

glemaitre changed the title ~~Add routing to LogisticRegressionCV~~ FEA Add routing to LogisticRegressionCV Jul 12, 2023

glemaitre changed the title ~~FEA Add routing to LogisticRegressionCV~~ ENH Add routing to LogisticRegressionCV Jul 12, 2023

glemaitre reviewed Jul 12, 2023

View reviewed changes

Omar Salman and others added 4 commits July 13, 2023 12:25

Address PR suggestions partially

5b723a0

Adjust and change the name of params in _check_method_params

c07a980

Resolve conflict in changelog

cc5ba48

Merge branch 'main' into logistic_cv_routing

915624a

glemaitre reviewed Jul 13, 2023

View reviewed changes

glemaitre self-requested a review July 13, 2023 18:22

OmarManzoor added 3 commits July 14, 2023 11:10

Update docstrings and value error

6772e5b

Test for the score method as well in test_lr_cv_scores_differ_when_sa…

49f7955

…mple_weight_is_requested

Minor formatting

7efe941

This comment was marked as outdated.

Sign in to view

Pass sample_weight to scorer in score method even in default case

a111bdd

glemaitre approved these changes Jul 14, 2023

View reviewed changes

glemaitre merged commit 02d20c1 into scikit-learn:main Jul 14, 2023
28 checks passed

OmarManzoor deleted the logistic_cv_routing branch July 17, 2023 10:27

lorentzenchr reviewed Nov 15, 2023

View reviewed changes

glemaitre mentioned this pull request Nov 15, 2023

DOC remove default parameter values for private function in logistic module #27787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add routing to LogisticRegressionCV #26525

ENH Add routing to LogisticRegressionCV #26525

OmarManzoor commented Jun 7, 2023 •

edited by adrinjalali

adrinjalali commented Jun 8, 2023

adrinjalali left a comment

OmarManzoor commented Jun 25, 2023

github-actions bot commented Jun 25, 2023 •

edited

adrinjalali left a comment

adrinjalali commented Jul 7, 2023

adrinjalali commented Jul 10, 2023

glemaitre Jul 12, 2023 •

edited

glemaitre Jul 13, 2023

OmarManzoor Jul 14, 2023

glemaitre Jul 14, 2023

glemaitre Jul 12, 2023

glemaitre Jul 12, 2023

glemaitre Jul 12, 2023

glemaitre Jul 12, 2023

glemaitre Jul 13, 2023

glemaitre Jul 13, 2023

glemaitre Jul 13, 2023

This comment was marked as outdated.

glemaitre left a comment

lorentzenchr Nov 15, 2023

		assert pytest.approx(lr_cv1.scores_[1]) != lr_cv2.scores_[1]


		def test_lr_cv_scores_without_enabling_metadata_routing():

	LogisticRegressionCV :meth:`fit` by checking the difference
	`LogisticRegressionCV.fit` by checking the difference

	"""Test sample_weight is correctly passed to the scorer in
	"""Test `sample_weight` is correctly passed to the scorer in

	in scores with the case when sample_weight is not requested.
	in scores with the case when `sample_weight` is not requested.

ENH Add routing to LogisticRegressionCV #26525

ENH Add routing to LogisticRegressionCV #26525

Conversation

OmarManzoor commented Jun 7, 2023 • edited by adrinjalali

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

adrinjalali commented Jun 8, 2023

adrinjalali left a comment

Choose a reason for hiding this comment

OmarManzoor commented Jun 25, 2023

github-actions bot commented Jun 25, 2023 • edited

✔️ Linting Passed

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali commented Jul 7, 2023

adrinjalali commented Jul 10, 2023

glemaitre Jul 12, 2023 • edited

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

This comment was marked as outdated.

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarManzoor commented Jun 7, 2023 •

edited by adrinjalali

github-actions bot commented Jun 25, 2023 •

edited

glemaitre Jul 12, 2023 •

edited