8000 [MRG + 1] update test_common.py deprecation warning by blakeflei · Pull Request #7024 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 1] update test_common.py deprecation warning #7024

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
Merged
merged 2 commits into from
Jul 17, 2016
Merged

[MRG + 1] update test_common.py deprecation warning #7024

merged 2 commits into from
Jul 17, 2016

Conversation

blakeflei
Copy link
Contributor

Reference Issue

Too much warnings during the testing process #7006

What does this implement/fix? Explain your changes.

sklearn.tests.test_common.test_get_params_invariance('RandomizedPCA', <class 'sklearn.d
ecomposition.pca.RandomizedPCA'>) ... /Users/bf/Desktop/Scipy2016/Sprints/scikit-learn/
scikit-learn/sklearn/utils/deprecation.py:52: DeprecationWarning: Class RandomizedPCA i
s deprecated; RandomizedPCA will be removed in 0.20. Use PCA(svd_solver='randomized') i
nstead. The new implementation DOES NOT store whiten components_. Apply transform to ge
t them.
warnings.warn(msg, category=DeprecationWarning)

Any other comments?

blakeflei
blakeflei reviewed Jul 16, 2016
View reviewed changes
sklearn/tests/test_common.py
@@ -222,7 +222,7 @@ def test_transformer_n_iter():
else:
yield check_transformer_n_iter, name, estimator


@ignore_warnings(category=DeprecationWarning)
Copy link
Contributor Author
@blakeflei blakeflei Jul 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so confident about this one - the test_get_params_invariance(): is more general than just RandomizedPCA.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, in the common tests we might allow that... Or maybe, can we inspect a class to see if it is deprecated? Below there is a hard-coded test for projected gradient. We should check that if the class is deprecated, we catch the deprecation warning, but not otherwise (it might call to deprecated functions, which we should fix).

Copy link
Contributor Author
@blakeflei blakeflei Jul 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RandomizedPCA class is also deprecated, but hard coding each class isn't best practice either... How would you recommend going about it - create a list of classes?
GaussianProcess and GMM also have the same warning, and are both deprecated:

from sklearn.mixture import GMM
hasattr(GMM.init,"deprecated_original")

True

from sklearn.gaussian_process import GaussianProcess
hasattr(GaussianProcess.init,"deprecated_original")

True

@amueller
Copy link
Member

Great. Have you checked that the warnings are actually caught?
LGTM.

@blakeflei
Copy link
Contributor Author

As far as I can tell, I think we're good on this issue:

sklearn.tests.test_common.test_get_params_invariance('RandomizedPCA', <class 'sklearn.decomposition.pca.RandomizedPCA'>) ... ok
sklearn.tests.test_common.test_get_params_invariance('GMM', <class 'sklearn.mixture.
gmm.GMM'>) ... ok
sklearn.tests.test_common.test_get_params_invariance('GaussianProcess', <class 'skle
arn.gaussian_process.gaussian_process.GaussianProcess'>) ... ok

@amueller amueller changed the title [WIP] update test_common.py deprecation warning [MRG + 1] update test_common.py deprecation warning Jul 17, 2016
@amueller
Copy link
Member

@tguillemot mrg?

@tguillemot
Copy link
Contributor

I clearly prefer that way of removing the deprecated classes.
+1

@amueller amueller merged commit 536b26a into scikit-learn:master Jul 17, 2016
@tguillemot
Copy link
Contributor

Thanks @blakeflei

@blakeflei
Copy link
Contributor Author

@tguillemot @amueller @juliathebrave Thanks for the support! I appreciate all the feedback.

@b-carter b-carter mentioned this pull request Aug 18, 2016
Merged
olologin pushed a commit to olologin/scikit-learn that referenced this pull request Aug 24, 2016
@blakeflei @olologin
[MRG + 1] update test_common.py deprecation warning (scikit-learn#7024)
c2b7945 
* update test_common.py deprecation warning

* Updated test_common.test_get_params_invariance to omit deprecation warnings on deprecated classes per issue 7006
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
@blakeflei @TomDLT
[MRG + 1] update test_common.py deprecation warning (scikit-learn#7024)
2d0525e 
* update test_common.py deprecation warning

* Updated test_common.test_get_params_invariance to omit deprecation warnings on deprecated classes per issue 7006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blakeflei @amueller @tguillemot
0