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

common tests: ``test_get_params_invariance`` invariance should be part of _yield_all_tests #7533

Open
amueller opened this Issue Sep 29, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@amueller
Member

amueller commented Sep 29, 2016

I think the test_get_params_invariance test should be removed and the check_get_params_invariance should be added to _yield_all_tests in estimator_checks.py.
This is less code and will also make this test run as part of check_estimator.

We might want to do something similar with the n_iter tests above.

@JungeAlexander

This comment has been minimized.

Show comment
Hide comment
@JungeAlexander

JungeAlexander Oct 8, 2016

Contributor

Looks like a feasible first contribution, I'd like to look into this.

Contributor

JungeAlexander commented Oct 8, 2016

Looks like a feasible first contribution, I'd like to look into this.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 8, 2016

Member

go for it

Member

jnothman commented Oct 8, 2016

go for it

JungeAlexander added a commit to JungeAlexander/scikit-learn that referenced this issue Oct 9, 2016

Test get_params invariance in common estimator tests
Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465
@JungeAlexander

This comment has been minimized.

Show comment
Hide comment
@JungeAlexander

JungeAlexander Oct 9, 2016

Contributor

Before looking into moving the *_n_iter tests, could someone please have a look at my commit linked above and tell me if I am roughly on the right track? Especially regarding the seemingly redundant test for the get_params attribute that I removed as mentioned in the commit message.

Contributor

JungeAlexander commented Oct 9, 2016

Before looking into moving the *_n_iter tests, could someone please have a look at my commit linked above and tell me if I am roughly on the right track? Especially regarding the seemingly redundant test for the get_params attribute that I removed as mentioned in the commit message.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 9, 2016

Member

I suppose that elsewhere we practically require estimators to have get_params, so I suppose that's fine. Your change LGTM.

Member

jnothman commented Oct 9, 2016

I suppose that elsewhere we practically require estimators to have get_params, so I suppose that's fine. Your change LGTM.

@JungeAlexander

This comment has been minimized.

Show comment
Hide comment
@JungeAlexander

JungeAlexander Oct 10, 2016

Contributor

Great, thank you for the feedback! I'll continue here.

Contributor

JungeAlexander commented Oct 10, 2016

Great, thank you for the feedback! I'll continue here.

JungeAlexander added a commit to JungeAlexander/scikit-learn that referenced this issue Oct 11, 2016

Move test_transformer_n_iter() to estimator_checks.py
Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533
@JungeAlexander

This comment has been minimized.

Show comment
Hide comment
@JungeAlexander

JungeAlexander Oct 11, 2016

Contributor

Commit above moves check_transformer_n_iter and I will soon continue with moving check_non_transformer_estimators_n_iter.

Contributor

JungeAlexander commented Oct 11, 2016

Commit above moves check_transformer_n_iter and I will soon continue with moving check_non_transformer_estimators_n_iter.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 13, 2016

Member

thanks @JungeAlexander. You can already create a pull request and keep iterating on that branch.

Member

amueller commented Oct 13, 2016

thanks @JungeAlexander. You can already create a pull request and keep iterating on that branch.

JungeAlexander added a commit to JungeAlexander/scikit-learn that referenced this issue Oct 15, 2016

Move test_non_transformer_estimators_n_iter() to estimator_checks.py
Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533
@JungeAlexander

This comment has been minimized.

Show comment
Hide comment
@JungeAlexander

JungeAlexander Oct 15, 2016

Contributor

Currently some tests fail. Some hints:

AttributeError: 'Isomap' object has no attribute 'n_iter_'
AttributeError: 'KernelPCA' object has no attribute 'n_iter_'
AttributeError: 'LocallyLinearEmbedding' object has no attribute 'n_iter_'
ValueError: For mono-task outputs, use ElasticNet (MultiTaskElasticNet)
ValueError: For mono-task outputs, use ElasticNetCV (MultiTaskElasticNetCV)
ValueError: For mono-task outputs, use ElasticNet (MultiTaskLasso)
ValueError: For mono-task outputs, use LassoCV (MultiTaskLassoCV)
AttributeError: 'OneClassSVM' object has no attribute 'n_iter_'

Will look into this later. Please let me know if you have a better idea than skipping the test for specifically those estimators where it fails right now

Contributor

JungeAlexander commented Oct 15, 2016

Currently some tests fail. Some hints:

AttributeError: 'Isomap' object has no attribute 'n_iter_'
AttributeError: 'KernelPCA' object has no attribute 'n_iter_'
AttributeError: 'LocallyLinearEmbedding' object has no attribute 'n_iter_'
ValueError: For mono-task outputs, use ElasticNet (MultiTaskElasticNet)
ValueError: For mono-task outputs, use ElasticNetCV (MultiTaskElasticNetCV)
ValueError: For mono-task outputs, use ElasticNet (MultiTaskLasso)
ValueError: For mono-task outputs, use LassoCV (MultiTaskLassoCV)
AttributeError: 'OneClassSVM' object has no attribute 'n_iter_'

Will look into this later. Please let me know if you have a better idea than skipping the test for specifically those estimators where it fails right now

JungeAlexander added a commit to JungeAlexander/scikit-learn that referenced this issue Oct 16, 2016

Fix check_non_transformer_estimators_n_iter calls
test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

amueller added a commit that referenced this issue Oct 20, 2016

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_

amueller added a commit to amueller/scikit-learn that referenced this issue Oct 25, 2016

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_

afiodorov added a commit to unravelin/scikit-learn that referenced this issue Apr 25, 2017

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_

Sundrique added a commit to Sundrique/scikit-learn that referenced this issue Jun 14, 2017

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_

paulha added a commit to paulha/scikit-learn that referenced this issue Aug 19, 2017

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this issue Nov 15, 2017

[MRG + 1] Move n_iter and get_params invariance tests to common estim…
…ator_checks (#7677)

* Test get_params invariance in common estimator tests

Remove test_get_params_invariance() from `test_common.py` and add
test call to _yield_all_tests() in `estimator_checks.py` to make
sure that get_params(deep=False) of a given Estimator returns a
subset of get_params(deep=True).

Compared to test_get_params_invariance(), it is NOT tested anymore
whether the given Estimator has an attribute get_params since
class BaseEstimator in `base.py` defines such an attribute
for each Estimator.

Partially addresses issue #7533
Also related to issue #4465

* Move test_transformer_n_iter() to estimator_checks.py

Remove the test test_transformer_n_iter() from tests/test_common.py
and perform the test logic in utils/estimator_checks.py instead.
Specifically, the method _yield_transformer_checks() now yields
check_transformer_n_iter() as part of the set of tests for
transformers.

test_transformer_n_iter() tests that that transformers with an
attribute max_iter, return the attribute of n_iter at least 1.

Partially addresses latter part of issue #7533

* Move test_non_transformer_estimators_n_iter() to estimator_checks.py

Remove the test_non_transformer_estimators_n_iter() from
tests/test_common.py; perform the test logic in
utils/estimator_checks.py instead.
Specifically, the method _yield_non_meta_checks() now yields
check_non_transformer_estimators_n_iter().

test_transformer_n_iter() tests that that estimators that are not
transformers with an attribute max_iter, return the attribute n_iter
of at least 1.

NOTE: The current implementation makes said test run for more
estimators than before this commit.
For some of these estimators, the test fails. This needs to be addressed
(see FIXME in line 111-115 of utils/estimator_checks.py for a potential
place to start).

Partially addresses latter part of issue #7533

* Fix check_non_transformer_estimators_n_iter calls

test_transformer_n_iter() test is now only run for
estimators where the test is applicable.

Partially addresses latter part of issue #7533

* Run check_non_transformer_estimators_n_iter on multi-class estimators

To do this, use helper method multioutput_estimator_convert_y_2d.
Also remove multi_output parameter from
check_non_transformer_estimators_n_iter since this parameter is not
used anywhere and corresponding cases should be handled by said
helper method.

Also, some pep8 line length fixes.

* Fix documentation for n_iter tests

There was some confusion between attributes and parameters.
Also rename n_iter to n_iter_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment