-
Notifications
You must be signed in to change notification settings - Fork 86
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
Removed incorrect parameter passed to pipeline classes in _add_baseline_pipelines
#941
Conversation
Codecov Report
@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 99.86% 99.87% +0.01%
==========================================
Files 170 170
Lines 8645 8640 -5
==========================================
- Hits 8633 8629 -4
+ Misses 12 11 -1
Continue to review full report at Codecov.
|
evalml/automl/automl_search.py
Outdated
strategy_dict = {"strategy": "mode"} | ||
baseline = ModeBaselineBinaryPipeline(parameters={"Baseline Classifier": strategy_dict}) | ||
elif self.problem_type == ProblemTypes.MULTICLASS: | ||
strategy_dict = {"strategy": "random_weighted"} | ||
strategy_dict = {"strategy": "mode"} | ||
baseline = ModeBaselineMulticlassPipeline(parameters={"Baseline Classifier": strategy_dict}) | ||
elif self.problem_type == ProblemTypes.REGRESSION: | ||
strategy_dict = {"strategy": "mean"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go with the classes we made and update the strategy dictionary. Alternative could be to create RandomWeightedBaselineBinaryPipeline etc 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just update the ModeBaselineBinaryPipeline
class definition with the right hyperparameters:
class ModeBaselineBinaryPipeline(BinaryClassificationPipeline):
"""Mode Baseline Pipeline for binary classification."""
custom_name = "Mode Baseline Binary Classification Pipeline"
component_graph = ["Baseline Classifier"]
custom_hyperparameters = {
"Baseline Classifier": {"strategy": ["mode"]}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freddyaboulton That's actually the definition of ModeBaselineBinaryPipeline
right now! As to why we were using strategy_dict
then... I'm going to have to refresh my memory 😂 . We could simply delete this and pass in no parameters, will circle back!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angela97lin This looks good to me. I have one question on the implementation but it's minor/non-blocking!
evalml/automl/automl_search.py
Outdated
strategy_dict = {"strategy": "mode"} | ||
baseline = ModeBaselineBinaryPipeline(parameters={"Baseline Classifier": strategy_dict}) | ||
elif self.problem_type == ProblemTypes.MULTICLASS: | ||
strategy_dict = {"strategy": "random_weighted"} | ||
strategy_dict = {"strategy": "mode"} | ||
baseline = ModeBaselineMulticlassPipeline(parameters={"Baseline Classifier": strategy_dict}) | ||
elif self.problem_type == ProblemTypes.REGRESSION: | ||
strategy_dict = {"strategy": "mean"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just update the ModeBaselineBinaryPipeline
class definition with the right hyperparameters:
class ModeBaselineBinaryPipeline(BinaryClassificationPipeline):
"""Mode Baseline Pipeline for binary classification."""
custom_name = "Mode Baseline Binary Classification Pipeline"
component_graph = ["Baseline Classifier"]
custom_hyperparameters = {
"Baseline Classifier": {"strategy": ["mode"]}
}
_add_baseline_pipelines
_add_baseline_pipelines
if num_pipelines == 0: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated changes, but since we added in baseline pipelines, this will never be true and is not covered by codecov, so removing to pass codecov!
Closes #935