FEA Add metadata routing to models that inherit from LinearModelCV by OmarManzoor · Pull Request #27478 · scikit-learn/scikit-learn

OmarManzoor · 2023-09-27T10:49:57Z

Reference Issues/PRs

Towards #22893

What does this implement/fix? Explain your changes.

Adds metadata routing to LassoCV, ElasticNetCV, MultiTaskElasticNetCV, MultiTaskLassoCV as all of them inherit from the base LinearModelCV.

Any other comments?

CC: @adrinjalali @glemaitre

github-actions · 2023-09-27T10:52:27Z

✔️ Linting Passed

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

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

OmarManzoor · 2023-09-27T12:44:24Z

@adrinjalali @glemaitre So I fixed the tests and made some adjustments. Could you kindly have a look to see if the overall approach and the corresponding additional test seems fine? If yes then we can add comments to refine this PR.

adrinjalali

I think we can do routing only for CV and let these classes be consumers of sample_weight, and the underlying fit of the sub-estimators don't accept anything other than sample_weight anyway.

adrinjalali · 2023-09-28T12:46:30Z

Also need to move the estimators from unsupported to supported in metadata_routing.rst

OmarManzoor · 2023-09-29T07:51:33Z

Thank you for the feedback @adrinjalali

adrinjalali

Nice!

adrinjalali · 2023-09-29T08:52:25Z

+        if sample_weight is not None and not has_fit_parameter(self, "sample_weight"):
+            raise ValueError("Underlying estimator does not support sample weights.")


this can't happen, can it? if sample weight is not in the signature, it cannot possibly be passed ;)

MultiTaskElasticNetCV and MultiTaskLassoCV do not support sample_weight currently. But now as we have added **params to both, technically someone can pass sample_weight and if they do and it is unchecked, the code might break unexpectedly. What do you think?

the thing is, sample_weight could in theory be used in the cv splitter. So we'd need to check, if the splitter doesn't use it, and the estimator also doesn't support it, then raise.

What happens if the splitter uses it? Then if we allow sample weight to be passed further than the splitter, the place where we fit the base/child model will pass sample weight as well and that won't work with these estimators. Do we then need to check this where we are using process routing? That is if the splitter.split does not contain sample weight and the estimator doesn't support it as well, then we raise. If the splitter has it but the estimator doesn't support it we set it to None after process routing.

…th don't support, if cv splitter supports but estimator doesn't after routing set sample weight to be None

…ple_weight

glemaitre · 2023-10-05T10:08:30Z

+            Parameters to be passed to the CV splitter.
+
+            .. versionadded:: 1.4
+                Only available if `enable_metadata_routing=True`. See


As in the other PR, I would suggest to add a reference to set_config and config_context to be explicit where enable_metadata_routing needs to be set.

This would need to be done in all places.

we can do this in a separate PR.

adrinjalali · 2023-10-05T12:13:33Z

+                or by using the context manager
+                    ``with config_context(enable_metadata_routing=True)``.


we shouldn't be advertising the context manager really. It'll cause a lot of issues in this case, if it's enabled in certain places of the code and not in other places.

adrinjalali

Awesome, thanks @OmarManzoor

glemaitre

LGTM as well.

…cikit-learn#27478)

FEA Add metadata routing to models that inherit from LinearModelCV

5167f18

github-actions Bot added the module:linear_model label Sep 27, 2023

Revert commented code

0f7380b

OmarManzoor commented Sep 27, 2023

View reviewed changes

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

OmarManzoor added 2 commits September 27, 2023 17:38

Fix and adjust tests

a599243

Minor refactor to remove extra diff

de5e501

Add changelog entry

13f1b55

adrinjalali reviewed Sep 28, 2023

View reviewed changes

Comment thread sklearn/linear_model/_coordinate_descent.py

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

adrinjalali mentioned this pull request Sep 28, 2023

SLEP006 - Metadata Routing task list #22893

Open

28 tasks

OmarManzoor added 2 commits September 29, 2023 12:47

Updates: PR suggestions-Only route for cv splitter

7db21f9

Doc fixes

33e19c0

OmarManzoor marked this pull request as ready for review September 29, 2023 07:51

adrinjalali reviewed Sep 29, 2023

View reviewed changes

OmarManzoor added 3 commits September 29, 2023 15:01

PR suggestions + add some further tests

865eda1

Updates: Raise for sample weight only if estimator and cv splitter bo…

572223c

…th don't support, if cv splitter supports but estimator doesn't after routing set sample weight to be None

Fix docstring error

16dfcb0

glemaitre self-requested a review October 2, 2023 13:48

OmarManzoor added 2 commits October 3, 2023 10:44

Fix codecov

af8d353

Merge branch 'main' into linear_model_cv_routing

6a89c94

adrinjalali reviewed Oct 5, 2023

View reviewed changes

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

Comment thread sklearn/utils/validation.py Outdated

OmarManzoor added 2 commits October 5, 2023 14:47

PR suggestions: use consumes method to check if splitter supports sam…

83ce735

…ple_weight

Merge branch 'main' into linear_model_cv_routing

260bb38

adrinjalali reviewed Oct 5, 2023

View reviewed changes

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated

Address PR suggestion

e8c149e

glemaitre reviewed Oct 5, 2023

View reviewed changes

Address further suggestions on PR

7d498f2

adrinjalali reviewed Oct 5, 2023

View reviewed changes

OmarManzoor added 2 commits October 5, 2023 17:25

Adjust doc

fcdc234

Improve wording

9a02f61

adrinjalali approved these changes Oct 9, 2023

View reviewed changes

adrinjalali requested a review from glemaitre October 9, 2023 10:46

adrinjalali mentioned this pull request Oct 9, 2023

FEA Add metadata routing to OrthogonalMatchingPursuitCV #27500

Merged

Merge branch 'main' into linear_model_cv_routing

fa6cdc7

glemaitre approved these changes Oct 10, 2023

View reviewed changes

glemaitre merged commit 9226441 into scikit-learn:main Oct 10, 2023

OmarManzoor deleted the linear_model_cv_routing branch October 10, 2023 08:18

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2023

FEA Add metadata routing to models that inherit from LinearModelCV (s…

19cbd13

…cikit-learn#27478)

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

FEA Add metadata routing to models that inherit from LinearModelCV (s…

f7e1c26

…cikit-learn#27478)

		if sample_weight is not None and not has_fit_parameter(self, "sample_weight"):
		raise ValueError("Underlying estimator does not support sample weights.")

		or by using the context manager
		``with config_context(enable_metadata_routing=True)``.

Uh oh!

Uh oh!

Conversation

OmarManzoor commented Sep 27, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

github-actions Bot commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Uh oh!

OmarManzoor commented Sep 27, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrinjalali commented Sep 28, 2023

Uh oh!

OmarManzoor commented Sep 29, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrinjalali Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

OmarManzoor Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

OmarManzoor Sep 29, 2023 • edited 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!

Uh oh!

Uh oh!

glemaitre Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

glemaitre Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

adrinjalali Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrinjalali Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

OmarManzoor Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

github-actions Bot commented Sep 27, 2023 •

edited

Loading

OmarManzoor Sep 29, 2023 •

edited

Loading

OmarManzoor Sep 29, 2023 •

edited

Loading