-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
Common test to check that __init__ doesn't mess with parameters #11117
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
Comments
__init__
doesn't mess with parameters
will that pass the clone checks too?!
|
That's a good question. Probably not. But @jorisvandenbossche's deprecation in the OneHotEncoder did something similar and it passed the tests?! |
As I said on that PR, the reason OneHotEncoder is passing the common tests, is because they are simply skipped ... |
Ah, sorry, I must have missed this. Either way, I think it would be worth testing this more explicitly in the common tests. |
omg I'm looking so forward to deleting most of clone in #9570 |
I was checking the 0.20 milestone and step in #7738. class Estimator(BaseEstimator):
def __init__(self, param=None):
self._param = param
@property
def param(self):
return some_stuff(self._param) Would it passed the tests? |
Well you'd need a setter as well. It's certainly not how we do things here. I'm not sure if it should be allowed |
So this, from sklearn.base import BaseEstimator
class DumbOne(BaseEstimator):
def __init__(self, param=0):
self.param =<
89CD
/span> 1
def fit(self, X, y):
self._validate_data(X, y)
return self
from sklearn.utils.estimator_checks import check_estimator
check_estimator(DumbOne()) gives this: /home/adrin/Projects/gh/me/scikit-learn/sklearn/utils/_array_api.py:123: UserWarning: Some scikit-learn array API features might rely on enabling SciPy's own support for array API to function properly. Please set the SCIPY_ARRAY_API=1 environment variable before importing sklearn or scipy. More details at: https://docs.scipy.org/doc/scipy/dev/api-dev/array_api.html
warnings.warn(
Traceback (most recent call last):
File "/tmp/1.py", line 12, in <module>
check_estimator(DumbOne())
File "/home/adrin/Projects/gh/me/scikit-learn/sklearn/utils/estimator_checks.py", line 681, in check_estimator
check(estimator)
File "/home/adrin/Projects/gh/me/scikit-learn/sklearn/utils/estimator_checks.py", line 3550, in check_parameters_default_constructible
assert param_value == init_param.default, failure_text
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Parameter param was mutated on init. All parameters must be stored unchanged. |
Uh oh!
There was an error while loading. Please reload this page.
There's
check_dont_overwrite_parameters
which checks thatfit
doesn't overwrite__init__
params, and there'scheck_no_attributes_set_in_init
that checks that only__init__
attributes are set. There is not check that they are actually set to the correct value, soactually passes the common tests. That's not good imho.
The text was updated successfully, but these errors were encountered: