8000 Common test to check that __init__ doesn't mess with parameters · Issue #11117 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
amueller opened this issue May 22, 2018 · 8 comments
Closed

Common test to check that __init__ doesn't mess with parameters #11117

amueller opened this issue May 22, 2018 · 8 comments
Labels
module:test-suite everything related to our tests Needs Decision Requires decision

Comments

@amueller
Copy link
Member
amueller commented May 22, 2018 •
edited
Loading

There's check_dont_overwrite_parameters which checks that fit doesn't overwrite __init__ params, and there's check_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, so

class DumbOne(BaseEstimator):
    def __init__(param=0):
        param = 1

actually passes the common tests. That's not good imho.
@amueller amueller changed the title Common test to check that __init__ doesn't mess with parameters Common test to check that __init__ doesn't mess with parameters May 22, 2018
@amueller amueller mentioned this issue May 22, 2018
Merged
@jnothman
Copy link
Member
jnothman commented May 22, 2018 via email

@amueller
Copy link
Member Author

That's a good question. Probably not. But @jorisvandenbossche's deprecation in the OneHotEncoder did something similar and it passed the tests?!

@jorisvandenbossche
Copy link
Member

As I said on that PR, the reason OneHotEncoder is passing the common tests, is because they are simply skipped ...
The current state of the PR certainly does not pass them (eg I also create attributes in the init that are not in the signature), but also before, the tests were already skipped for OneHotEncoder for other reasons (there are a few other tests failing unrelated to the init)

@amueller
Copy link
Member Author

Ah, sorry, I must have missed this. Either way, I think it would be worth testing this more explicitly in the common tests. clone is a weird way to test that, right?
At least there should be a test in test_common_tests.py to make sure the common tests detect that.

@amueller
Copy link
Member Author

omg I'm looking so forward to deleting most of clone in #9570

@glemaitre
Copy link
Member

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?

@jnothman
Copy link
Member

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

@cmarmo cmarmo added module:test-suite everything related to our tests Needs Decision Requires decision labels Jan 15, 2022
@adrinjalali
Copy link
Member

So this,

from sklearn.base import BaseEstimator

class DumbOne(BaseEstimator):
    def __init__(self, param=0):
        self.param = 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module:test-suite everything related to our tests Needs Decision Requires decision
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jnothman @amueller @jorisvandenbossche @cmarmo @adrinjalali @glemaitre
0