FIX methods in model_selection/_validation accept params=None with metadata routing enabled #30451
Conversation
…tadata routing enabled
| ], | ||
| ) | ||
| @config_context(enable_metadata_routing=True) | ||
| def test_cross_validate_params_none(func, extra_args): |
There was a problem hiding this comment.
Can we add a docstring for the test? It would be difficult to understand what it's testing later otherwise.
Or maybe, we can join this test with the next one (test_passed_unrequested_metadata) and modify the docstring there.
There was a problem hiding this comment.
Perfect, thank you. :)
| @@ -1636,6 +1637,7 @@ def permutation_test_score( | |||
| params = _check_params_groups_deprecation(fit_params, params, groups, "1.8") | |||
|
|
|||
| X, y, groups = indexable(X, y, groups) | |||
| params = params or {} | |||
There was a problem hiding this comment.
This one seems redundant since we're calling _check_params_groups_deprecation just before, which already does this. I don't think params can be None here
There was a problem hiding this comment.
Wouldn't it make sense to leave it there, because the call to _check_params_groups_deprecation will be removed later?
There was a problem hiding this comment.
we'd put it there when we remove that function. And we don't have to worry about it breaking code, cause the test will fail once we remove it.
| params = {} if params is None else params | ||
| params = params or {} |
There was a problem hiding this comment.
Why change? The original is more specific in only replacing None, and not things like {} and the various other things that are false-y.
(same for the other two instances of this in this PR)
There was a problem hiding this comment.
it's always a dictionary anyway
| @@ -1172,6 +1172,7 @@ def cross_val_predict( | |||
| """ | |||
| _check_groups_routing_disabled(groups) | |||
| X, y = indexable(X, y) | |||
| params = params or {} | |||
There was a problem hiding this comment.
We want to pass params to process_routing later, which motivates the "check if this is None", but can we not find a way to pass it to process_routing without having to have this line everywhere? Mostly because I think we will keep forgetting it :-/ and because it is repetitive.
The only way I can think of is to not use **params but something like kw_params=params and then checking for None in process_routing. Which isn't a super solution, but maybe someone smarter has a good idea?
There was a problem hiding this comment.
these are only needed in functions, not where we have estimators and routing from their methods. There these are passed to the method as **kwargs and therefore never None. We have very few functions where we accept a params arg and then route those. As you can see in the implementation of the routing here, it's quite convoluted and the routing machinery is not designed for these functions. So I think this single line is not adding any complexity compared to how we've implemented routing here.
Fixes #30447
This fixes an issue with functions in
model_selection/_validation.pywhere they'd raise ifparams=Noneand metadata routing is enabled.cc @StefanieSenger @OmarManzoor @jeremiedbb