-
Notifications
You must be signed in to change notification settings - Fork 83
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
Delete make_pipeline_from_components
#2218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2218 +/- ##
=========================================
- Coverage 100.0% 100.0% -0.0%
=========================================
Files 287 287
Lines 24451 24399 -52
=========================================
- Hits 24433 24381 -52
Misses 18 18
Continue to review full report at Codecov.
|
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.
Looks great @angela97lin ! I love the net deletion of lines hehe
@@ -144,46 +145,6 @@ def make_pipeline(X, y, estimator, problem_type, parameters=None, custom_hyperpa | |||
return base_class(complete_component_graph, parameters=parameters, custom_hyperparameters=custom_hyperparameters) | |||
|
|||
|
|||
def make_pipeline_from_components(component_instances, problem_type, custom_name=None, random_seed=0): |
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'm debating whether we should keep this around for another release and instead issue a deprecation warning in this pr.
I'm ok with deleting this util, I don't think it got a lot of traction from users but just noting that I think we use a different process for deleting functions/methods.
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.
Interesting point, but I'm okay deleting immediately, since I think the main use of this was internal alignment :p Plus, it's not too difficult to do what make_pipeline_from_components
with our new pipelines so not too much lost there!
comparison_pipeline = logistic_regression_multiclass_pipeline_class(parameters={"Logistic Regression Classifier": {"n_jobs": 1}}) | ||
objective = 'Log Loss Multiclass' | ||
elif problem_type == ProblemTypes.REGRESSION: | ||
X, y = X_y_regression | ||
base_pipeline_class = RegressionPipeline | ||
stacking_component_name = StackedEnsembleRegressor.name | ||
input_pipelines = [make_pipeline_from_components([regressor], problem_type) for regressor in stackable_regressors] | ||
input_pipelines = [base_pipeline_class([regressor]) for regressor in stackable_regressors] |
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.
nit-pick: I don't think there's a need for base_pipeline_class
anymore.
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.
Hmmm, I think we still need it for the line after (depending on which problem type, we create a specific pipeline class) but I'm okay with updating this line to just use RegressionPipeline so it's more clear! 😁
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.
LGTM! Agree with Freddy that it might be useful to deprecate before immediately removing from EvalML
Closes #2173