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

Cleanup y_predictions in score of Pipeline functions #711

Merged
merged 36 commits into from
Apr 28, 2020

Conversation

angela97lin
Copy link
Contributor

Cleans up some code in score of pipelines

@angela97lin angela97lin self-assigned this Apr 24, 2020
@angela97lin angela97lin marked this pull request as draft April 24, 2020 19:18
@codecov
Copy link

codecov bot commented Apr 26, 2020

Codecov Report

Merging #711 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #711   +/-   ##
=======================================
  Coverage   99.07%   99.07%           
=======================================
  Files         140      140           
  Lines        4992     4992           
=======================================
  Hits         4946     4946           
  Misses         46       46           

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 4c40d8e...4c40d8e. Read the comment docs.

@angela97lin angela97lin marked this pull request as ready for review April 27, 2020 15:29
Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

The change looks good. Will approve once we figure out that codecov thing

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

Looks great!

@angela97lin I see the following when I run git grep -ni "notimplemented" on master:

.coveragerc:7:    raise NotImplementedError
docs/source/changelog.rst:227:    * Pipelines now require an estimator as the last component in ``component_list``. Slicing pipelines now throws an ``NotImplementedError`` to avoid returning pipelines without an estimator.
evalml/objectives/objective_base.py:12:        raise NotImplementedError("This objective must have a `name` attribute as a class variable")
evalml/objectives/objective_base.py:19:        raise NotImplementedError("This objective must have a `greater_is_better` boolean attribute as a class variable")
evalml/objectives/objective_base.py:27:        raise NotImplementedError("This objective must have a `score_needs_proba` boolean attribute as a class variable")
evalml/objectives/objective_base.py:41:        raise NotImplementedError("`objective_function` must be implemented.")
evalml/pipelines/components/component_base.py:20:        return NotImplementedError("This component must have `name` as a class variable.")
evalml/pipelines/components/component_base.py:27:        return NotImplementedError("This component must have `model_family` as a class variable.")
evalml/pipelines/components/estimators/estimator.py:14:        return NotImplementedError("This component must have `supported_problem_types` as a class variable.")
evalml/pipelines/pipeline_base.py:36:        return NotImplementedError("This pipeline must have `component_graph` as a class variable.")
evalml/pipelines/pipeline_base.py:118:            raise NotImplementedError('Slicing pipelines is currently not supported.')
evalml/pipelines/pipeline_base.py:125:        raise NotImplementedError('Setting pipeline components is not supported.')
evalml/tests/component_tests/test_components.py:144:            raise NotImplementedError()
evalml/tests/pipeline_tests/test_pipelines.py:184:    with pytest.raises(NotImplementedError, match=setting_err_msg):
evalml/tests/pipeline_tests/test_pipelines.py:188:    with pytest.raises(NotImplementedError, match=slicing_err_msg):
evalml/tuners/tuner.py:20:        raise NotImplementedError
evalml/tuners/tuner.py:33:        raise NotImplementedError
evalml/tuners/tuner.py:42:        raise NotImplementedError

If we're going to go with this pattern, which I'm liking, we should make this change for all relevant callsites, right? It would be great if you did that in this PR, but filing an issue to track a separate PR is fine too.

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.

3 participants