FEA Metadata routing for VotingClassifier and VotingRegressor #27584

StefanieSenger · 2023-10-14T14:23:03Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Adds metadata routing to VotingClassifier. The challenge here was that it takes a list of (name, est) tuples as an init argument instead of only an estimator. I have modified test_metaestimators_metadata_routing.py and _metadata_requests.py for handling this.

Any other comments?

The main question is, if the modifications of the test and the routing file should stay as they are.

All tests pass as it is (except for the old tests for VotingRegressor, StackingClassifier and StackingRegressor). These three have to get their routing implemented in the same PR, I think, because they all share a common function (_fit_single_estimator) with VotingClassifier. They also take a list of (name, est) tuples instead of a single estimator.

I'll wait with this until test_metaestimators_metadata_routing.py and _metadata_requests.py look as they should.

github-actions · 2023-10-14T14:24:42Z

✔️ Linting Passed

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

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

adrinjalali

Wouldn't change the test_metaestimators_metadata_routing.py and _metadata_requests.py. You can write tests the same way as done for ColumnTransformer and Pipeline.

sklearn/ensemble/_base.py

sklearn/ensemble/_voting.py

sklearn/ensemble/_base.py

sklearn/ensemble/_voting.py

sklearn/tests/test_metaestimators_metadata_routing.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

sklearn/ensemble/tests/test_voting.py

sklearn/tests/metadata_routing_common.py

doc/whats_new/v1.4.rst

sklearn/ensemble/_voting.py

sklearn/ensemble/tests/test_voting.py

sklearn/tests/metadata_routing_common.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

StefanieSenger

I went through your comments, @adrinjalali, and applied your suggestions (except for one, that I didn't know how to).
Would you have a look?
Thanks a lot so far. :)

sklearn/ensemble/_voting.py

sklearn/tests/metadata_routing_common.py

sklearn/ensemble/tests/test_voting.py

adrinjalali · 2024-01-22T11:16:29Z

sklearn/ensemble/_voting.py

+    def _more_tags(self):
+        return {"preserves_dtype": []}


would be nice to have a comment as why this is needed.

To be honest, I don't remember anymore and cannot re-create the error that made me put it.
So, I think I should leave it away then.

sklearn/ensemble/_voting.py

sklearn/ensemble/tests/test_voting.py

adrinjalali · 2024-01-22T12:32:13Z

sklearn/ensemble/tests/test_voting.py

+
+    with pytest.raises(ValueError, match=re.escape(error_message)):
+        est.fit(X, y, sample_weight=sample_weight, metadata=metadata)
+


Also need a test to check get_metadata_routing works before calling fit:

@pytest.mark.usefixtures("enable_slep006") @pytest.mark.parametrize( "Estimator, Child", [(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)], ) def test_get_metadata_routing_without_fit(Estimator, Child): # Test that get_metadata_routing() doesn't raise when called before fit. est = Estimator([("sub_est", Child())]) est.get_metadata_routing()

Why would we need that test here specifically? It seems like a test for testing get_metadata_routing(), not a test for the routing in VotingClassifier and VotingRegressor.

It's a test for this specific implementation of get_metadata_routing for this estimator. It is to avoid this bug: #28239

StefanieSenger · 2024-02-01T09:41:00Z

I made the improvements according to your review, @adrinjalali.
Would you take another look? What do you think about the preserves_dtype tag? All common tests pass without it...

StefanieSenger · 2024-02-05T12:04:37Z

sklearn/ensemble/tests/test_voting.py

+    [(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],
+)
+def test_get_metadata_routing_without_fit(Estimator, Child):
+    # Test that metadata_routing() doesn't raise when called before fit.


Suggested change

# Test that metadata_routing() doesn't raise when called before fit.

"""Test that get_metadata_routing() works regardless of the Child's

consumption of any metadata."""

Maybe this helps to explain why this test is here.

I cannot fully see the connection with #28239, can you please explain, @adrinjalali?

The core of that issue was that this method would fail if the sub-estimator didn't support any metadata at all. So we test it here.

@adrinjalali
I can see that in issue #28239 the routing failed on fit, when the sub-estimator cannot handle any sample weights.
Your solution from #28240 was to make sure sample_weight is getting to be part for fit_params before the routing is started and you added a test (test_non_consuming_estimator_works) that tests exactly that.
VotingClassifier is not covered by that test, but it surely needs a similar test. I still cannot see how test_get_metadata_routing_without_fit would test for the same case as test_non_consuming_estimator_works.

the right PR: https://github.com/scikit-learn/scikit-learn/pull/28188/files

StefanieSenger · 2024-02-05T12:11:31Z

sklearn/ensemble/tests/test_voting.py

+@pytest.mark.usefixtures("enable_slep006")
+@pytest.mark.parametrize(
+    "Estimator, Child",
+    [(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],


Suggested change

[(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],

[(VotingClassifier, NonConsumingClassifier), (VotingRegressor, NonConsumingRegressor)],

After #28240 is merged, to be more explicit.

#28240 is merged, but I'm not sure if I understand your comment.

adrinjalali

Looking good.

adrinjalali · 2024-02-06T13:02:59Z

sklearn/ensemble/tests/test_voting.py

+@pytest.mark.usefixtures("enable_slep006")
+@pytest.mark.parametrize(
+    "Estimator, Child",
+    [(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],


#28240 is merged, but I'm not sure if I understand your comment.

adrinjalali · 2024-02-06T13:04:16Z

sklearn/ensemble/tests/test_voting.py

+    [(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],
+)
+def test_get_metadata_routing_without_fit(Estimator, Child):
+    # Test that metadata_routing() doesn't raise when called before fit.


The core of that issue was that this method would fail if the sub-estimator didn't support any metadata at all. So we test it here.

adrinjalali

LGTM.

adam2392

Thank you for this informative PR!

I leveraged some of these tests in #28432, which is good since the tests are almost 100% re-usable and test the Bagging* classes similarly.

sklearn/ensemble/_stacking.py

sklearn/ensemble/tests/test_voting.py

glemaitre

LGTM. I will apply the nitpicks and make the auto-merge

routing for VotingClassifier

ab14402

github-actions bot added module:ensemble module:utils labels Oct 14, 2023

adrinjalali reviewed Oct 17, 2023

View reviewed changes

sklearn/ensemble/_base.py Outdated Show resolved Hide resolved

sklearn/ensemble/_voting.py Outdated Show resolved Hide resolved

sklearn/ensemble/_voting.py Outdated Show resolved Hide resolved

StefanieSenger added 3 commits October 19, 2023 21:40

routing for all the classifiers VotingClassifier uses

c44b362

routing done in parents fit method

7a3e0fa

routing for VotingRegressor

b06403f

adrinjalali reviewed Oct 24, 2023

View reviewed changes

StefanieSenger and others added 3 commits October 24, 2023 16:13

immunity for Stacking* and changes after review

ecea0ad

Update sklearn/tests/test_metaestimators_metadata_routing.py

ebcd1be

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

changes after review

16c52cd

StefanieSenger changed the title ~~FEA Metadata routing for VotingClassifier~~ FEA Metadata routing for VotingClassifier and VotingRegressor Oct 24, 2023

StefanieSenger and others added 3 commits October 24, 2023 20:45

Merge branch 'main' into routing_VotingClassifier

0b63677

added custom test for Voting*

3b4c6a3

revert list-support for tests

a6691df

StefanieSenger commented Oct 27, 2023

View reviewed changes

sklearn/ensemble/tests/test_voting.py Show resolved Hide resolved

StefanieSenger commented Oct 27, 2023

View reviewed changes

sklearn/tests/metadata_routing_common.py Outdated Show resolved Hide resolved

adrinjalali reviewed Jan 4, 2024

View reviewed changes

StefanieSenger and others added 6 commits January 5, 2024 11:35

Update sklearn/ensemble/_voting.py

7fe675a

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Update sklearn/ensemble/_voting.py

5ada54a

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Merge branch 'main' into routing_VotingClassifier

c85858c

Update sklearn/ensemble/_voting.py

82bea9a

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Update sklearn/ensemble/tests/test_voting.py

fd91818

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

changes after review

5489d31

StefanieSenger commented Jan 5, 2024

View reviewed changes

sklearn/ensemble/_voting.py Show resolved Hide resolved

sklearn/tests/metadata_routing_common.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_voting.py Outdated Show resolved Hide resolved

ignore FutureWarning

5784c56

adrinjalali reviewed Jan 22, 2024

View reviewed changes

improvements according to review

277793f

Merge branch 'main' into routing_VotingClassifier

1fbe65f

StefanieSenger commented Feb 5, 2024

View reviewed changes

adrinjalali reviewed Feb 6, 2024

View reviewed changes

adrinjalali approved these changes Feb 13, 2024

View reviewed changes

adrinjalali added the Waiting for Second Reviewer First reviewer is done, need a second one! label Feb 13, 2024

adam2392 mentioned this pull request Feb 15, 2024

[FEA] Add metadata routing to BaggingClassifier and BaggingRegressor #28432

Merged

adam2392 reviewed Feb 16, 2024

View reviewed changes

glemaitre self-requested a review February 22, 2024 15:48

Merge remote-tracking branch 'origin/main' into pr/StefanieSenger/27584

c375792

glemaitre approved these changes Feb 22, 2024

View reviewed changes

sklearn/ensemble/_stacking.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_voting.py Outdated Show resolved Hide resolved

glemaitre approved these changes Feb 22, 2024

View reviewed changes

nitpicks

d7d14c7

glemaitre enabled auto-merge (squash) February 22, 2024 16:46

glemaitre merged commit 77a63e7 into scikit-learn:main Feb 22, 2024

StefanieSenger deleted the routing_VotingClassifier branch February 22, 2024 17:40

glemaitre mentioned this pull request May 16, 2024

SLEP006 - Metadata Routing task list #22893

Open

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEA Metadata routing for VotingClassifier and VotingRegressor #27584

FEA Metadata routing for VotingClassifier and VotingRegressor #27584

StefanieSenger commented Oct 14, 2023 •

edited

Loading

github-actions bot commented Oct 14, 2023 •

edited

Loading

adrinjalali left a comment

StefanieSenger left a comment

adrinjalali Jan 22, 2024

StefanieSenger Feb 1, 2024

adrinjalali Jan 22, 2024

StefanieSenger Feb 1, 2024

adrinjalali Feb 2, 2024

StefanieSenger commented Feb 1, 2024

StefanieSenger Feb 5, 2024 •

edited

Loading

adrinjalali Feb 6, 2024

StefanieSenger Feb 7, 2024 •

edited

Loading

adrinjalali Feb 13, 2024

StefanieSenger Feb 5, 2024

adrinjalali Feb 6, 2024

adrinjalali left a comment

adrinjalali Feb 6, 2024

adrinjalali Feb 6, 2024

adrinjalali left a comment

adam2392 left a comment

glemaitre left a comment


		with pytest.raises(ValueError, match=re.escape(error_message)):
		est.fit(X, y, sample_weight=sample_weight, metadata=metadata)

	# Test that metadata_routing() doesn't raise when called before fit.
	"""Test that get_metadata_routing() works regardless of the Child's
	consumption of any metadata."""

	[(VotingClassifier, ConsumingClassifier), (VotingRegressor, ConsumingRegressor)],
	[(VotingClassifier, NonConsumingClassifier), (VotingRegressor, NonConsumingRegressor)],

FEA Metadata routing for VotingClassifier and VotingRegressor #27584

FEA Metadata routing for VotingClassifier and VotingRegressor #27584

Conversation

StefanieSenger commented Oct 14, 2023 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

github-actions bot commented Oct 14, 2023 • edited Loading

✔️ Linting Passed

adrinjalali left a comment

Choose a reason for hiding this comment

StefanieSenger 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

StefanieSenger commented Feb 1, 2024

StefanieSenger Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanieSenger Feb 7, 2024 • edited Loading

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

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

StefanieSenger commented Oct 14, 2023 •

edited

Loading

github-actions bot commented Oct 14, 2023 •

edited

Loading

StefanieSenger Feb 5, 2024 •

edited

Loading

StefanieSenger Feb 7, 2024 •

edited

Loading