Use one sampler in split DefaultAlgorithm pipelines
#3696
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3696 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 339 339
Lines 34431 34451 +20
=======================================
+ Hits 34304 34324 +20
Misses 127 127
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jeremyliweishih
requested review from
eccabay,
chukarsten,
christopherbunn and
fjlanasa
and removed request for
eccabay
evalml/pipelines/utils.py
Outdated
| @@ -742,6 +743,7 @@ def _make_pipeline_from_multiple_graphs( | |||
| pipeline_name (str): Custom name for the final pipeline. | |||
| sub_pipeline_names (Dict): Dictionary mapping original input pipeline names to new names. This will be used to rename components. Defaults to None. | |||
| prior_components (Dict): Component graph of components preceding the split of multiple graphs. Must be in component graph format, {"Label Encoder": ["Label Encoder", "X", "y"]} and currently restricted to components that only alter X input. | |||
| pre_estimator_components (Dict): Component graph of components before the estimator after the split of multiple graphs. Must be in component graph format, {"Label Encoder": ["Label Encoder", "X", "y"]} and currently restricted to components that only alter X input. | |||
There was a problem hiding this comment.
I like this implementation - it's a smooth way of hooking up these sorts of components into the component graph. Slightly overengineered for the problem at hand, but will make our lives easier if/when we need to add more pre-estimator components in the future. Do you have any sort of plan for how we'll do that if that becomes the case? Since right now, we just have the one sampler and we set the input X and y knowing they'll be reset as the first_pre_estimator_component, but what would this look like with more components?
I don't think this is a blocking question, just something to keep in mind and maybe file an issue to track.
There was a problem hiding this comment.
this implementation should work with more components if pre_estimator_components is a a component graph that hooks all the pre_estimator_components together. For example I believe the impl still would work if we have:
{sampler.name: [sampler.name, "X", "y"], "StandardScaler": ["StandardScaler, "sampler.X", "Sampler.y"]}
There was a problem hiding this comment.
+1 for this implementation, I think this will be super useful in the future. I'm okay with merging it as-is, but we might want to consider updating the parameter names for prior_components and pre_estimator_components to be a bit more precise? Maybe something with a pre/post scheme like pre_pipeline_components/post_pipelines_components.
There was a problem hiding this comment.
will update @christopherbunn
evalml/pipelines/utils.py
Outdated
| @@ -742,6 +743,7 @@ def _make_pipeline_from_multiple_graphs( | |||
| pipeline_name (str): Custom name for the final pipeline. | |||
| sub_pipeline_names (Dict): Dictionary mapping original input pipeline names to new names. This will be used to rename components. Defaults to None. | |||
| prior_components (Dict): Component graph of components preceding the split of multiple graphs. Must be in component graph format, {"Label Encoder": ["Label Encoder", "X", "y"]} and currently restricted to components that only alter X input. | |||
| pre_estimator_components (Dict): Component graph of components before the estimator after the split of multiple graphs. Must be in component graph format, {"Label Encoder": ["Label Encoder", "X", "y"]} and currently restricted to components that only alter X input. | |||
There was a problem hiding this comment.
+1 for this implementation, I think this will be super useful in the future. I'm okay with merging it as-is, but we might want to consider updating the parameter names for prior_components and pre_estimator_components to be a bit more precise? Maybe something with a pre/post scheme like pre_pipeline_components/post_pipelines_components.
Fixes #3076.