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
Update pipeline docs after API change #2195
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2195 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 296 296
Lines 24712 24712
=======================================
Hits 24694 24694
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.
Very cool! 😁
@@ -187,7 +189,8 @@ | |||
"metadata": {}, | |||
"source": [ | |||
"## Pipeline Parameters\n", | |||
"You can also pass in custom parameters, which will then be used when instantiating each component in `component_graph`. The parameters dictionary needs to be in the format of a two-layered dictionary where the key-value pairs are the component name and corresponding component parameters dictionary. The component parameters dictionary consists of (parameter name, parameter values) key-value pairs.\n", | |||
"\n", | |||
"You can also pass in custom parameters by using the `parameters` parameter, which will then be used when instantiating each component in `component_graph`. The parameters dictionary needs to be in the format of a two-layered dictionary where the key-value pairs are the component name and corresponding component parameters dictionary. The component parameters dictionary consists of (parameter name, parameter values) key-value pairs.\n", |
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.
Love it
"with open('pipeline.pkl', 'rb') as f:\n", | ||
" pickled_pipeline = pickle.load(f)\n", | ||
"\n", | ||
"pickled_pipeline.fit(X, y)" |
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.
This is great. I would add
assert pickled_pipeline == pipeline_to_pickle
before the call to fit
, to demonstrate the two pipeline instances are equivalent.
" - `TimeSeriesMulticlassClassificationPipeline`\n", | ||
" \n", | ||
"The class you want to use will depend on your problem type.\n", | ||
"The only required parameter input for instantiating a pipeline instance is `component_graph`, which is either a list or a dictionary containing a sequence of components to be fit and evaluated.\n", |
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.
💯
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 to me!
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 perfect!
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.
This is good work. Nothing blocking here - just the comment on how I'm personally not sure what's going on with component_obj = None and what you're trying to show there. Nice. I do like this documentation a lot.
"\n", | ||
" def __init__(self, parameters, random_seed=0):\n", | ||
" super().__init__(self.component_graph, parameters=parameters, random_seed=random_seed)\n" | ||
"component_graph_as_list = ['Imputer', 'Random Forest Classifier']\n", |
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.
This looks a lot simpler.
"metadata": { | ||
"scrolled": 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.
what is this? lol, does it mean it starts scrolled down?
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.
Apparently means the cell was scrolled yeah lol I'm gonna delete this 😂
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.
Just curious: does that change how the docs look on RTD?
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.
Not that I could tell... 🤔
" parameters = {\"parameter_1\": parameter_1}\n", | ||
" super().__init__(parameters=parameters,\n", | ||
" component_obj=transformer,\n", | ||
" component_obj=None,\n", |
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.
Is this no longer the method of making a third party transformer? I think this change is a little confusing because it begs the question "what is the component_obj param?"
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.
Ahaha previously it didn't use a real third party transformer but it never error-ed because the code wasn't run... now that's no longer the case 😓 I can update this to use a real third party transformer and then comment it as such!
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.
Actually, we have info on this on our components doc which is linked so I'll just remove this line so it's not a focus.
Closes #2158