Add skopt.space.Categorical documentation and tests for component hyperparameter range #1228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1228 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 200 200
Lines 12339 12365 +26
=======================================
+ Hits 12330 12356 +26
Misses 9 9
Continue to review full report at Codecov.
|
bchen1116
requested review from
dsherry,
angela97lin,
jeremyliweishih,
christopherbunn,
eccabay and
freddyaboulton
There was a problem hiding this comment.
@bchen1116 This looks good! I think it would be good to write a test where AutoMLSearch searches over pipelines that use a component that defines some hyperparameter with Categorical before merging. (I think we could subclass LogisticRegressionClassifier and change the range for penalty for example)
There was a problem hiding this comment.
Looks good! I agree with @freddyaboulton's comments about testing / clarification but otherwise 👍
| @@ -852,3 +853,36 @@ def test_component_equality_all_components(component_class): | |||
| parameters = component.parameters | |||
| equal_component = component_class(**parameters) | |||
| assert component == equal_component | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("categorical", [{ | |||
| } | ||
|
|
||
| automl = AutoMLSearch(problem_type="multiclass", allowed_pipelines=[CustomPipeline]) | ||
| automl.search(X, y) |
There was a problem hiding this comment.
@bchen1116 just a tip, for tests which don't check scoring, we usually mock fit/score so that we save time in the unit test. See some of the other tests in this file
@patch('evalml.pipelines.BinaryClassificationPipeline.score')
@patch('evalml.pipelines.BinaryClassificationPipeline.fit')
def test_categorical_hyperparam(mock_fit, mock_score, X_y_multi):There was a problem hiding this comment.
Oh I see. So patching fit/score without calling mock_fit or mock_score still makes it run faster?
| random_state=0) | ||
|
|
||
| assert MockComponent(agg_type="mean").fit(X, y) | ||
| assert MockComponent(agg_type="moat", category="blue").fit(X, y) |
There was a problem hiding this comment.
@bchen1116 why call fit here? What is the goal of this test?
There was a problem hiding this comment.
The goal was to make sure the component works, but since the component uses an estimator, I called fit to satisfy codecov
There was a problem hiding this comment.
@bchen1116 thanks, great! I left a couple minor test suggestions.
fix #352
Update documentation to include option for either list or
skopt.space.Categoricalin the hyperparameter definition. Include unit tests.