-
Notifications
You must be signed in to change notification settings - Fork 89
Replace pipelines' supported_problem_types' with 'problem_type' in base classes #678
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #678 +/- ##
==========================================
+ Coverage 99.07% 99.08% +0.01%
==========================================
Files 139 139
Lines 4963 4933 -30
==========================================
- Hits 4917 4888 -29
+ Misses 46 45 -1
Continue to review full report at Codecov.
|
7fc2597
to
346f339
Compare
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 just one typo!
@@ -24,13 +24,12 @@ | |||
"objective = 'Precision_Macro'\n", | |||
"\n", | |||
"\n", | |||
"# pipeline needs to be a subclass of `PipelineBase`\n", | |||
"class CustomPipeline(PipelineBase):\n", | |||
"# the pipeline needs to be a subclass of one of our base pipelines, in this case `BinaryClassificationPipeline`\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.
typo here - should be multiclass
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.
Ah good catch, thanks!
Fixes #668
Now that we have
BinaryClassificationPipeline
,MultiClassificationPipeline
andRegressionPipeline
, we don't need pipelines to support a list of problem types. The base classes can set the problem type for internal use, but otherwise users don't need to know about it.