-
Notifications
You must be signed in to change notification settings - Fork 87
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
Separate parameters
and describe
in PipelineBase
#501
Conversation
@@ -299,6 +289,20 @@ def graph(self, filepath=None): | |||
""" | |||
return make_pipeline_graph(self.component_graph, self.name, filepath=filepath) | |||
|
|||
@property |
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 will change the behavior of PipelineBase.parameters
from the parameters dictionary the user provides into the parameters of the initialized components. After #500 the two behaviors should be equivalent.
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.
Cool!
…s_362_describe
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
=======================================
Coverage 98.43% 98.44%
=======================================
Files 105 105
Lines 3450 3469 +19
=======================================
+ Hits 3396 3415 +19
Misses 54 54
Continue to review full report at Codecov.
|
parameters
and describe
in PipelineBase
parameters
and describe
in PipelineBase
'Simple Imputer': { | ||
'impute_strategy': 'median' | ||
}, | ||
'Logistic Regression Classifier': { | ||
'penalty': 'l2', | ||
'C': 3.0, | ||
'random_state': 1 |
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.
How come this went away in this PR?
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.
Since the parameters property is taken from the initialized components instead of the parameters dictionary parameters such as random_state don't show up anymore! When #500 gets fixed it should edit this test to add it back in.
@jeremyliweishih do you have a separate issue for doing the same thing for |
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 added a couple suggestions, but LGTM!
As mentioned, if we don't have a separate issue yet for doing the same thing for components, we should either file an issue or also do that in this PR.
|
fixes #362.