FIX Removes validation in __init__ for Pipeline #21888
Conversation
Co-authored-by: arisayosh <15692997+arisayosh@users.noreply.github.com> Co-authored-by: iofall <50991099+iofall@users.noreply.github.com>
…tion Co-authored-by: arisayosh <15692997+arisayosh@users.noreply.github.com> Co-authored-by: iofall <50991099+iofall@users.noreply.github.com>
There was a problem hiding this comment.
Thank for the pull request! Can you please update the tests to check that the exceptions can only be raised in the fit method below?
Also document this change in the doc/whats_new/v1.1.rst by taking example to other PRs that address part of #21406.
sklearn/tests/test_pipeline.py
Outdated
| @@ -168,17 +168,17 @@ def predict_log_proba(self, X, got_attribute=False): | |||
| def test_pipeline_init(): | |||
| # Test the various init parameters of the pipeline. | |||
| with pytest.raises(TypeError): | |||
| Pipeline() | |||
| Pipeline().fit([[1]], [1]) | |||
There was a problem hiding this comment.
Can you please rewrite those test to make it explicit that the error is raised in fit?
pipeline = Pipeline()
with pytest.raises(TypeError):
pipeline.fit([[1]], [1])There was a problem hiding this comment.
Hi @ogrisel, thank you for reviewing. Would you like me to edit so that the above is done for each instance of pipeline in the test file?
|
Thank you @ogrisel for the review. Will fix the changes and document them in that file. What are your thoughts on creating a new test function for those that are tested in .fit()? 8000 |
That's a very good point. |
to changelog Co-authored-by: arisayosh <15692997+arisayosh@users.noreply.github.com> Co-authored-by: iofall <50991099+iofall@users.noreply.github.com>
| @@ -165,20 +165,23 @@ def predict_log_proba(self, X, got_attribute=False): | |||
| return self | |||
|
|
|||
|
|
|||
| def test_pipeline_init(): | |||
| # Test the various init parameters of the pipeline. | |||
| def test_pipeline_invalid_parameters(): | |||
There was a problem hiding this comment.
Renamed function since we are not validating parameters during instantiation. We decided not to split the function in two because it caused code repetition.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
This was a combined effort with @iofall! Thank you for the reviews @ogrisel and @thomasjpfan :) |
Reference Issues/PRs
Relates to issue #21406
What does this implement/fix? Explain your changes.
__init__by removingself.__validate_steps()test_pipeline_initfunction intest_pipeline.pyto test in.fit()instead of__init__Any other comments?
Curious if we should instead create another function
test_pipeline_fitfor our changes since we are testing some of the parameters duringfitand notinit.