[BUG] fix the RandomizedSearchCV backend API mismatch.#1015
[BUG] fix the RandomizedSearchCV backend API mismatch.#1015MaazAli8460 wants to merge 2 commits intosktime:mainfrom
RandomizedSearchCV backend API mismatch.#1015Conversation
If scoring= None, set it to CRPS().
Added backend parameter in RandomizedSearchCV constructor.
There was a problem hiding this comment.
Pull request overview
Fixes the documented backend parameter mismatch in RandomizedSearchCV so callers can select a parallel backend as intended.
Changes:
- Exposes
backendinRandomizedSearchCV.__init__and forwards it toBaseGridSearch. - Sets a default scoring metric (
CRPS) whenscoring=Noneto avoid runtime failures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # scoring = check_scoring(self.scoring, obj=self) | ||
| scoring = self.scoring | ||
| if scoring is None: | ||
| scoring = CRPS() |
There was a problem hiding this comment.
scoring is documented as accepting str/callable, but _fit assumes a metric object with .name and .get_tag() (used for scoring_name and ranking). Passing a callable or string will raise at runtime. Consider coercing/validating self.scoring to a BaseMetric (or raising a clear ValueError if unsupported) so the implementation matches the documented API.
| scoring = CRPS() | |
| scoring = CRPS() | |
| # validate scoring: BaseGridSearch requires a metric-like object | |
| # with at least a ``name`` attribute and ``get_tag`` method for | |
| # result naming and ranking. Passing a bare string or callable will | |
| # otherwise fail at runtime. | |
| if not (hasattr(scoring, "name") and hasattr(scoring, "get_tag")): | |
| raise ValueError( | |
| f"'scoring' must be a metric-like object with 'name' and " | |
| f"'get_tag' attributes when used with {self.__class__.__name__}. " | |
| "Passing strings or generic callables is not supported " | |
| "in this tuner; please pass a metric object instead." | |
| ) |
| verbose=verbose, | ||
| return_n_best_estimators=return_n_best_estimators, | ||
| backend=backend, | ||
| error_score=error_score, |
There was a problem hiding this comment.
Please add a small regression test that instantiates RandomizedSearchCV(..., backend="threading"/"loky") and asserts the attribute is set and used (e.g., by checking no error and that backend is forwarded into parallelize via a simple fit). This prevents future regressions of the documented backend parameter support.
fkiraly
left a comment
There was a problem hiding this comment.
Thanks - to prevent regression, could you add a parameter set to get_test_params that sets backend explicitly to be the "none" backend (to avoid nested parallelization)?
RandomizedSearchCV backend API mismatch.
Reference Issues/PRs
Fixes #1013.
What does this implement/fix? Explain your changes.
This PR fixes the RandomizedSearchCV backend API mismatch.
Changes made:
Added backend="loky" to RandomizedSearchCV.init in _tuning.py
Passed backend=backend through to the BaseGridSearch constructor so backend selection works as documented.
Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
I did not add tests in the commit, but this script is running without errors.
Output
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
skproroot directory (not theCONTRIBUTORS.md). Common badges:code- fixing a bug, or adding code logic.doc- writing or improving documentation or docstrings.bug- reporting or diagnosing a bug (get this pluscodeif you also fixed the bug in the PR).maintenance- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.python_dependenciestag and ensureddependency isolation, see the estimator dependencies guide.