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

[MRG] Deprecates gaussian process regression_models and correlation_models. #9717

Merged
merged 5 commits into from Oct 16, 2017

Conversation

Projects
None yet
5 participants
@thechargedneutron
Contributor

thechargedneutron commented Sep 8, 2017

Reference Issue

Fixes #9714

What does this implement/fix? Explain your changes.

Removes all the occurrences of regression_models and correlation_models.

Any other comments?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller
Member

amueller commented Sep 8, 2017

you need to deprecate them, not remove them: http://scikit-learn.org/dev/developers/contributing.html#deprecation

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 8, 2017

Contributor

@amueller Since regression_models is python file rather than a method or a class, do we need to add deprecation at the top of the file or before each and every function of regression_models ?

Contributor

thechargedneutron commented Sep 8, 2017

@amueller Since regression_models is python file rather than a method or a class, do we need to add deprecation at the top of the file or before each and every function of regression_models ?

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 9, 2017

Contributor

@amueller Added deprecation warning before all the functions of the required files. Can you please review and point out the reason for test failure? I am not able to figure out the reason for this failure.
ERROR: /home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_common.py.test_non_meta_estimators:check_estimators_pickle(GaussianProcess).

Contributor

thechargedneutron commented Sep 9, 2017

@amueller Added deprecation warning before all the functions of the required files. Can you please review and point out the reason for test failure? I am not able to figure out the reason for this failure.
ERROR: /home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_common.py.test_non_meta_estimators:check_estimators_pickle(GaussianProcess).

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Sep 10, 2017

Member

@thechargedneutron you need to capture warning in test suite now and remove these functions from examples and narrative doc.

Member

agramfort commented Sep 10, 2017

@thechargedneutron you need to capture warning in test suite now and remove these functions from examples and narrative doc.

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 10, 2017

Contributor

@agramfort So, we have check_estimators_pickle which is causing the failure in case of GaussianProcess, but we already have @ignore_warnings above this function. What is it doing?
I understand something needs to be changed before yield check_estimators_pickle but can't figure out. Can you help? :(

Contributor

thechargedneutron commented Sep 10, 2017

@agramfort So, we have check_estimators_pickle which is causing the failure in case of GaussianProcess, but we already have @ignore_warnings above this function. What is it doing?
I understand something needs to be changed before yield check_estimators_pickle but can't figure out. Can you help? :(

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 14, 2017

Contributor

I am not able to figure out the exact change I need to make in order to avoid this error. Using @ignore_warnings above fit in GaussianProcess raises an error in check_fit_score_takes_y (The error arises here: args = [p.name for p in signature(func).parameters.values()]. I tried quite a few things but could not avoid the error. :( Help!!

Contributor

thechargedneutron commented Sep 14, 2017

I am not able to figure out the exact change I need to make in order to avoid this error. Using @ignore_warnings above fit in GaussianProcess raises an error in check_fit_score_takes_y (The error arises here: args = [p.name for p in signature(func).parameters.values()]. I tried quite a few things but could not avoid the error. :( Help!!

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 15, 2017

Member
Member

jnothman commented Sep 15, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 17, 2017

Member

You're currently failing on a doctest in doc/modules/gaussian_process.rst. I think that entire section on Legacy Gaussian Processes should be deleted. You can do so in this PR.

Member

jnothman commented Sep 17, 2017

You're currently failing on a doctest in doc/modules/gaussian_process.rst. I think that entire section on Legacy Gaussian Processes should be deleted. You can do so in this PR.

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 17, 2017

Contributor

Yes, but I have skipped the pickling test for GaussianProcess. I don't feel that's acceptable. I was getting pickling error (please see an earlier commit failure log). If this is acceptable, I would do the rest in no time. :)

Contributor

thechargedneutron commented Sep 17, 2017

Yes, but I have skipped the pickling test for GaussianProcess. I don't feel that's acceptable. I was getting pickling error (please see an earlier commit failure log). If this is acceptable, I would do the rest in no time. :)

@jnothman

Hmmm... A deprecated public function should in theory be picklable if it's built the right way (since it can be reconstructed as a fixed-named attribute of a module); but it probably isn't built the right way. Sorry if that's not a very helpful comment.

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 17, 2017

Contributor

Can you point out the mistake in last commit? Why is it unpickable? I guess I need to catch warning somewhere, but I could not find where to.

Contributor

thechargedneutron commented Sep 17, 2017

Can you point out the mistake in last commit? Why is it unpickable? I guess I need to catch warning somewhere, but I could not find where to.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 18, 2017

Member

See #9787 which should resolve your issue

Member

jnothman commented Sep 18, 2017

See #9787 which should resolve your issue

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 18, 2017

Member

But please also do remove the legacy documentation from gaussian_process.rst

Member

jnothman commented Sep 18, 2017

But please also do remove the legacy documentation from gaussian_process.rst

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 18, 2017

Member

#9787 has been merged, so you can put the pickling test back

Member

jnothman commented Sep 18, 2017

#9787 has been merged, so you can put the pickling test back

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Sep 18, 2017

Member

LGTM assuming tests pass!

Member

jnothman commented Sep 18, 2017

LGTM assuming tests pass!

@thechargedneutron thechargedneutron changed the title from [WIP] Deprecates gaussian process regression_models and correlation_models. to [MRG] Deprecates gaussian process regression_models and correlation_models. Sep 18, 2017

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 22, 2017

Contributor

@amueller Review please.

Contributor

thechargedneutron commented Sep 22, 2017

@amueller Review please.

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Sep 30, 2017

Contributor

This needs a second review. Please @amueller or @agramfort or @lesteve . Thanks

Contributor

thechargedneutron commented Sep 30, 2017

This needs a second review. Please @amueller or @agramfort or @lesteve . Thanks

@thechargedneutron

This comment has been minimized.

Show comment
Hide comment
@thechargedneutron

thechargedneutron Oct 12, 2017

Contributor

@jnothman Could this deprecation be added in 0.19.1?

Contributor

thechargedneutron commented Oct 12, 2017

@jnothman Could this deprecation be added in 0.19.1?

@jnothman jnothman added this to the 0.19.1 milestone Oct 16, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 16, 2017

Member

Merging as trivial.

Member

jnothman commented Oct 16, 2017

Merging as trivial.

@jnothman jnothman merged commit 01d8a34 into scikit-learn:master Oct 16, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.17%)
Details
codecov/project 96.17% (+<.01%) compared to 7400775
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Oct 16, 2017

@thechargedneutron thechargedneutron deleted the thechargedneutron:deprecate_regression_models branch Oct 17, 2017

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 17, 2017

Member

@jnothman given that this will be merged in 0.19.1, the deprecation messages are misleading: they say deprecated in 0.20 removed in 0.22.

Member

lesteve commented Oct 17, 2017

@jnothman given that this will be merged in 0.19.1, the deprecation messages are misleading: they say deprecated in 0.20 removed in 0.22.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 18, 2017

Member

@jnothman I cherry-picked 99eadd4 in master. I assume you left the removal in 0.22 intentionally to err on the conservative side (I don't think it matters that much to be honest).

Member

lesteve commented Oct 18, 2017

@jnothman I cherry-picked 99eadd4 in master. I assume you left the removal in 0.22 intentionally to err on the conservative side (I don't think it matters that much to be honest).

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 18, 2017

Member
Member

jnothman commented Oct 18, 2017

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

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment