TST add a few more tests to API checks #29832
Conversation
|
Fail is bogus, need #29834 to fix the issues here. |
sklearn/utils/estimator_checks.py
Outdated
| yield check_estimator_repr | ||
| yield check_no_attributes_set_in_init | ||
| yield check_fit_score_takes_y | ||
| yield check_estimators_overwrite_params | ||
| yield check_dont_overwrite_parameters | ||
| yield check_estimators_fit_returns_self | ||
| yield partial(check_estimators_fit_returns_self, readonly_memmap=True) |
There was a problem hiding this comment.
Would something like a separate function be good _yield_joblib_compatability_checks or would that be confusing?
My impression upon skimming the code is that some of these checks ensure joblib compatability, but might also ensure some API compliance?
There was a problem hiding this comment.
Right, since this has to do with parallelisation, it's certainly API since it's needed for our meta-estimators. Creating a separate test name for it.
sklearn/utils/estimator_checks.py
Outdated
| yield check_estimator_repr | ||
| yield check_no_attributes_set_in_init | ||
| yield check_fit_score_takes_y | ||
| yield check_estimators_overwrite_params | ||
| yield check_dont_overwrite_parameters | ||
| yield check_estimators_fit_returns_self | ||
| yield partial(check_estimators_fit_returns_self, readonly_memmap=True) |
There was a problem hiding this comment.
Right, since this has to do with parallelisation, it's certainly API since it's needed for our meta-estimators. Creating a separate test name for it.
| @@ -753,10 +753,6 @@ def test_check_estimator(): | |||
| msg = "object has no attribute 'fit'" | |||
| with raises(AttributeError, match=msg): | |||
| check_estimator(BaseEstimator()) | |||
| # check that fit does input validation | |||
| msg = "Did not raise" | |||
There was a problem hiding this comment.
now that we assert the right message, it became clear that this was already tested in another test and was redundant.
|
I think I've addressed questions and suggestions. Ready for another review. |
|
@OmarManzoor maybe? |
This adds a few more tests to our API checks.
I'm not 100% sure about these two though:
WDYT @glemaitre @adam2392 ?