-
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
Rename _name
to custom_name
for pipelines
#650
Conversation
Codecov Report
@@ Coverage Diff @@
## master #650 +/- ##
=======================================
Coverage 98.97% 98.97%
=======================================
Files 136 136
Lines 4692 4694 +2
=======================================
+ Hits 4644 4646 +2
Misses 48 48
Continue to review full report at Codecov.
|
Documentation doesn't clearly show that |
@@ -230,7 +230,7 @@ class TestNamePipeline(PipelineBase): | |||
supported_problem_types = ['binary'] | |||
|
|||
class TestDefinedNamePipeline(PipelineBase): | |||
_name = "Cool Logistic Regression" | |||
custom_name = "Cool Logistic Regression" |
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 the only usage of custom_name
in our unit tests? That's surprising. Could you double-check that we have adequate coverage? For example we could add a test here which calls assert TestDefinedNamePipeline.custom_name == "Cool Logistic Regression"
or something. What does it show up as when we try to call pipeline.custom_name
for a pipeline which doesn't define a custom name? Good to have this sort of coverage in case we change the impl in the future.
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.
If we call for a pipeline that doesn't have custom_name it should error out. I can change it to return None
.
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! I added a couple notes but otherwise ready to 🚢
fixes #533.