Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX EmptyRequest.get defaults to Bunch of METHODS #28371

Merged
15 changes: 15 additions & 0 deletions sklearn/metrics/tests/test_score_objects.py
Expand Up @@ -1490,3 +1490,18 @@ def test_make_scorer_deprecation(deprecated_params, new_params, warn_msg):
assert deprecated_roc_auc_scorer(classifier, X, y) == pytest.approx(
roc_auc_scorer(classifier, X, y)
)


@pytest.mark.parametrize("enable_metadata_routing", [True, False])
def test_metadata_routing_multimetric_metadata_routing(enable_metadata_routing):
"""Test multimetric scorer works with and without metadata routing enabled when
there is no actual metadata to pass.

Non-regression test for https://github.com/scikit-learn/scikit-learn/issues/28256
"""
X, y = make_classification(n_samples=50, n_features=10, random_state=0)
estimator = EstimatorWithFitAndPredict().fit(X, y)

multimetric_scorer = _MultimetricScorer(scorers={"acc": get_scorer("accuracy")})
with config_context(enable_metadata_routing=enable_metadata_routing):
multimetric_scorer(estimator, X, y)
22 changes: 22 additions & 0 deletions sklearn/tests/test_metadata_routing.py
eddiebergman marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -239,6 +239,28 @@ class InvalidObject:
process_routing(InvalidObject(), "fit", groups=my_groups)


def test_process_routing_empty_params_get_with_default():
empty_params = {}
routed_params = process_routing(ConsumingClassifier(), "fit", **empty_params)

# Behaviour should be an empty dictionary returned for each method when retrieved.
for method in METHODS:
# This behaviour should be equivalent with using `get` with no default
params_for_method = routed_params[method]

# An empty dictionary for each method
assert isinstance(params_for_method, dict)
assert set(params_for_method.keys()) == set(METHODS)

default_params_for_method = routed_params.get(method)
assert default_params_for_method == params_for_method

# However, with a default, should return that instead.
assert routed_params.get(method, default="default") == "default"
assert routed_params.get(method, default=[]) == []
Copy link
Member

Choose a reason for hiding this comment

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

The test is failing but I don't think that we are using the sentinel in the file so it looks normal :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, never actually committed that change. I'm doing this PR in between little bits of free time. Apologies for the mini mistakes

assert routed_params.get(method, default=None) is None


def test_simple_metadata_routing():
# Tests that metadata is properly routed

Expand Down
8 changes: 7 additions & 1 deletion sklearn/utils/_metadata_requests.py
Expand Up @@ -115,6 +115,9 @@

METHODS = SIMPLE_METHODS + list(COMPOSITE_METHODS.keys())

# Used as a sentinel value to indicate nothing was passed.
_MISSING = object()


def _routing_enabled():
"""Return whether metadata routing is enabled.
Expand Down Expand Up @@ -1530,7 +1533,10 @@ def process_routing(_obj, _method, /, **kwargs):
# an empty dict on routed_params.ANYTHING.ANY_METHOD.
class EmptyRequest:
def get(self, name, default=None):
return default if default else {}
if not default:
return Bunch(**{method: dict() for method in METHODS})

return default
Copy link
Member

Choose a reason for hiding this comment

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

codecov is not happy here. I need to figure out when is it the case that default=None

Copy link
Member

Choose a reason for hiding this comment

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

@adrinjalali I assume that we should be able to cover this one because it would be equivalent to call e.g.

routed_params = _process_routing(self, "score", **kwargs)
routed_params.get("score", default="default")

I don't where is the best place to test this. This looks like a metadata routing test to me.


def __getitem__(self, name):
return Bunch(**{method: dict() for method in METHODS})
Expand Down