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.
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
LGTM. Thanks @adrinjalali
| 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 = fit_params | ||
|
|
||
| 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
There was a problem hiding this comment.
I don't understand this answer. If the user didn't pass any params then we want to replace None with {}. Which in Python is {} if params is None else params, so I don't understand why you want to make the change to something that is less precise in expressing the intention
There was a problem hiding this comment.
I directly pushed to revert this change to unblock this PR and the 1.6.1 release. In this context both formulations would have the same behavior, but I agree that we should use the one that expresses the behavior we want as most precisely and unambiguously as possible.
…tadata routing enabled (scikit-learn#30451) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
…tadata routing enabled (#30451) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
5 participants
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