Skip to content
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

Add supported_problem_types as required field for Pipelines and Components #515

Merged
merged 6 commits into from Mar 27, 2020

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Mar 20, 2020

Fixes #377.

@jeremyliweishih jeremyliweishih changed the title Add supported_problem_types as required field for Pipelines and Components [WIP] Add supported_problem_types as required field for Pipelines and Components Mar 20, 2020
@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #515 into master will increase coverage by 0.40%.
The diff coverage is 95.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
+ Coverage   98.26%   98.67%   +0.40%     
==========================================
  Files         113      113              
  Lines        3972     3985      +13     
==========================================
+ Hits         3903     3932      +29     
+ Misses         69       53      -16     
Impacted Files Coverage Δ
...valml/pipelines/components/estimators/estimator.py 80.00% <83.33%> (+1.05%) ⬆️
evalml/pipelines/pipeline_base.py 98.45% <85.71%> (ø)
evalml/pipelines/classification/catboost.py 100.00% <100.00%> (ø)
...ml/pipelines/classification/logistic_regression.py 100.00% <100.00%> (ø)
evalml/pipelines/classification/random_forest.py 100.00% <100.00%> (ø)
evalml/pipelines/classification/xgboost.py 100.00% <100.00%> (ø)
...ents/estimators/classifiers/catboost_classifier.py 100.00% <100.00%> (ø)
...ents/estimators/classifiers/logistic_regression.py 100.00% <100.00%> (ø)
...components/estimators/classifiers/rf_classifier.py 100.00% <100.00%> (ø)
...nents/estimators/classifiers/xgboost_classifier.py 100.00% <100.00%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3988eab...bb18a7b. Read the comment docs.

@jeremyliweishih jeremyliweishih self-assigned this Mar 20, 2020
@jeremyliweishih jeremyliweishih changed the title [WIP] Add supported_problem_types as required field for Pipelines and Components Add supported_problem_types as required field for Pipelines and Components Mar 20, 2020
@jeremyliweishih jeremyliweishih requested a review from dsherry Mar 20, 2020
def problem_types(cls):
return NotImplementedError("This pipeline must have `problem_types` as a class variable.")
def supported_problem_types(cls):
return NotImplementedError("This pipeline must have `supported_problem_types` as a class variable.")
Copy link
Collaborator

@dsherry dsherry Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. This field is deleted by @angela97lin 's feature branch! Heads up @angela97lin this rename will cause conflicts, but they shouldn't be too bad since your branch is just deleting this :)

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'm planning on merging this and #516 after @angela97lin's feature branch so we can take turns dealing with the conflicts! Is that ok @dsherry?

Copy link
Collaborator

@dsherry dsherry Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremyliweishih oh, sure. If you do that, it will only contain changes for components, because PipelineBase.problem_types will be deleted. But that sounds fine to me!

@@ -78,7 +79,7 @@ def test_describe_component():

def test_missing_attributes(X_y):
class MockComponentName(ComponentBase):
model_family = None
model_family = ModelFamily.NONE
Copy link
Collaborator

@dsherry dsherry Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Glad we added this to the enum 👍

@@ -28,6 +29,7 @@ Changelog
* Components and Pipelines now have a `model_family` field instead of `model_type`
* `get_pipelines` utility function now accepts `model_families` as an argument instead of `model_types`
* `PipelineBase.name` no longer returns structure of pipeline and has been replaced by `PipelineBase.summary`
* `PipelineBase.problem_types` and `Estimator.problem_types` has been renamed to `supported_problem_types`
Copy link
Collaborator

@dsherry dsherry Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. I think @angela97lin 's feature branch merge should delete this from the breaking changes list, because, well, it deletes the field entirely 😂

Copy link
Collaborator

@dsherry dsherry left a comment

@jeremyliweishih looks good to me!

One question for you before merge: have you checked that you got all the call-sites? I scanned through git grep -n "problem_types" and didn't see any.

I would suggest we should make the same update for ObjectiveBase and subclasses, but I happen to know @angela97lin's work will delete ObjectiveBase.problem_types as well, so 👍

@jeremyliweishih
Copy link
Contributor Author

jeremyliweishih commented Mar 24, 2020

@dsherry thanks for the review! Just checked again, should be good with all references to problem_types other than objectives.

@jeremyliweishih jeremyliweishih merged commit 89f7eae into master Mar 27, 2020
1 of 2 checks passed
@dsherry dsherry deleted the js_377_spt branch Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add supported_problem_types as a required static variable for Estimators
2 participants