[MRG + 1] deprecated randomized_l1 #9031

Sentient07 · 2017-06-07T07:52:37Z

This PR attempts to fix #8995

Sentient07 · 2017-06-07T07:55:38Z

@agramfort I'm not sure how the deprecation should be done, I added warning at the top of the file that is deprecated. @GaelVaroquaux asked me a gist of the code that has been removed, but I didn't remove any for now, rather in the warning message, said it would be removed in the next release

agramfort · 2017-06-07T07:59:35Z

@Sentient07 read this http://scikit-learn.org/stable/developers/contributing.html#deprecation

you'll also need to remove reference to this code in narrative documentation

jnothman · 2017-06-07T08:09:47Z

see cross_validation for another case of module-level deprecation

…

On 7 Jun 2017 5:59 pm, "Alexandre Gramfort" ***@***.***> wrote: @Sentient07 <https://github.com/sentient07> read this http://scikit-learn.org/stable/developers/contributing.html#deprecation you'll also need to remove reference to this code in narrative documentation — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64zx1fUkMt-z0S65sFRGxoUoyLt5ks5sBlhogaJpZM4NyVKr> .

Sentient07 · 2017-06-07T22:42:29Z

Sorry, had a long day of work and travel couldn't get back earlier. I had removed the deprecated module's code from the example section and specified the versions in which this module will be deprecated and removed.

agramfort · 2017-06-08T08:21:55Z

@Sentient07 there should be no warning when running the test of building the doc

agramfort · 2017-06-08T08:22:19Z

since you won't remove the tests now it means you need to capture the warnings

Sentient07 · 2017-06-08T09:35:51Z

Understood. But is it preferred to remove the tests ? ( cross_validation still seems to have its test, but raising warnings when the module is trying to be imported)

Sentient07 · 2017-06-08T10:52:39Z

Also, should I get rid of the examples/linear_model/plot_sparse_recovery module? I am not sure if it would be of much use once we get rid of the RandomisedLasso

agramfort · 2017-06-08T10:58:18Z

keep the tests but capture warning and remove examples and narrative doc about it

Sentient07 · 2017-06-08T13:45:26Z

@agramfort I have caught and ignored the warnings in test. I tested with no-capture option, and I don't see any warnings while running the tests. Also, I have removed the example and the related documentations. (I am not very sure what is narrative documentation)

jnothman · 2017-06-08T23:04:01Z

narrative documentation is basically anything in doc/ On 8 Jun 2017 11:45 pm, "Ramana Subramanyam" <notifications@github.com> wrote: @agramfort <https://github.com/agramfort> I have caught and ignored the warnings in test. I test with no-capture option and I don't see any warning. Also, I have removed the example and the related documentation. (I am not very sure what is narrative documentation) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6zI2ONvSN0fEovb7sez-OPY-hb6Eks5sB_r4gaJpZM4NyVKr> .

jnothman · 2017-06-14T11:55:32Z

flake8 failures:

Running flake8 on the diff in the range 623ee42..158cda7 (3 commit(s)):
--------------------------------------------------------------------------------
./sklearn/linear_model/randomized_l1.py:34:16: E127 continuation line over-indented for visual indent
               DeprecationWarning)
               ^
./sklearn/linear_model/tests/test_randomized_l1.py:20:80: E501 line too long (81 > 79 characters)
                                                    RandomizedLogisticRegression)
                                                                               ^

Sentient07 · 2017-06-14T23:38:12Z

Sorry about the flake8 errors, I have fixed that and squashed the commits into one.

jnothman · 2017-06-14T23:56:07Z

It seems I misled you. We shouldn't be doing a module deprecation like for cross_validation. That's done where people import the model directly. We should be deprecating each of the public objects in the module, like at sklearn/mixture/gmm.py. Apologies.

…

On 15 June 2017 at 09:38, Ramana Subramanyam ***@***.***> wrote: Sorry about the flake8 errors, I have fixed that and squashed the commits into one. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6-NHJP2QcoghIufVrn_4SjdG1deTks5sEG7mgaJpZM4NyVKr> .

Sentient07 · 2017-06-15T20:32:32Z

No problem, I will do the changes tomorrow.

amueller · 2017-06-19T16:07:34Z

@Sentient07 are you still on this? Usually I wouldn't be nagging, but we're trying to move towards a release ;)

Sentient07 · 2017-06-19T16:20:03Z

Will send a PR by tonight! Sorry, was having a deadline and couldn't find time before :(

…

On 19 Jun 2017 9:38 pm, "Andreas Mueller" ***@***.***> wrote: @Sentient07 <https://github.com/sentient07> are you still on this? Usually I wouldn't be nagging, but we're trying to move towards a release ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9031 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6IcFqIzCI83YR2bSd1ebGPnzODCkWoks5sFpzwgaJpZM4NyVKr> .

Sentient07 · 2017-06-20T12:39:32Z

@amueller sorry for the delay, I made the deprecation similar to GMM, fixed the merge conflict.

amueller

Looks good, but you need to fix the flake8 errors for travis to pass. Also, can you please add a test that checks that a warning is actually raised when instantiating an object of the class with assert_warns_message? Thanks!

amueller · 2017-06-20T16:00:41Z

sklearn/linear_model/__init__.py

-from .randomized_l1 import (RandomizedLasso, RandomizedLogisticRegression,
-                            lasso_stability_path)
+
+with warnings.catch_warnings():


you don't need to catch here, importing will not raise a deprecation warning

Ah! Forgot to remove

amueller · 2017-06-20T16:02:20Z

Oh also please add an entry to whats_new.rst

Sentient07 · 2017-06-20T21:10:50Z

@amueller I made the changes that you suggested. I did not test it locally as after the recent rebase, I am getting stuck with this error, ImportError: cannot import name bincount. Google results suggest me to update numpy but that did not work for me. I am pushing leaving the tests to Travis as I don't want this to postpone the release. Apologies.

jnothman

And if you otherwise think your work here is complete and want a full review, change the title's WIP to MRG.

jnothman · 2017-06-20T23:44:08Z

sklearn/linear_model/__init__.py

@@ -30,8 +30,7 @@
 from .passive_aggressive import PassiveAggressiveClassifier
 from .passive_aggressive import PassiveAggressiveRegressor
 from .perceptron import Perceptron
-from .randomized_l1 import (RandomizedLasso, RandomizedLogisticRegression,


We still need this. It is the source of test failures.

massich · 2017-06-28T13:35:58Z

@Sentient07, this is a blocker issue.
I think there is not much to be done in order to finish it. If you don't have the time to fix it soon, ping me and I'll take over.

Sentient07 · 2017-06-28T14:06:23Z

I had pushed a commit before and it didn't get updated here. I have pushed it again. Once Travis passes, I'll change it to mrg

amueller · 2017-06-29T16:12:30Z

doc/whats_new.rst

@@ -1279,6 +1279,9 @@ Model evaluation and meta-estimators
     the parameter ``n_labels`` is renamed to ``n_groups``.
     :issue:`6660` by `Raghav RV`_.

+   - The :mod:`sklearn.linear_model.randomized_l1` is deprecated.
+     :issue: `8995` by `Andreas Mueller`_.


The "by" refers to the contributor, so you. You can use the :user: pattern as elsewhere in whatsnew.

There should be a list of deprecated functions in 0.19 in the whatsnew.

amueller

LGTM apart from whatsnew nitpicks.

amueller · 2017-06-29T16:12:48Z

sklearn/linear_model/__init__.py

@@ -30,8 +30,10 @@
 from .passive_aggressive import PassiveAggressiveClassifier
 from .passive_aggressive import PassiveAggressiveRegressor
 from .perceptron import Perceptron
+


why the change?

Probably when I added code for catching the exception at the time of import, after removing it, this empty line came up

amueller · 2017-06-29T16:13:25Z

sklearn/linear_model/randomized_l1.py

@@ -58,6 +60,8 @@ def _resample_model(estimator_func, X, y, scaling=.5, n_resampling=200,
    return scores_


+@deprecated("The class BaseRandomizedLinearModel is deprecated in 0.19"


Hm this should not have been public, but given that it was, deprecating it is the right thing to do.

amueller · 2017-06-29T16:13:52Z

doc/whats_new.rst

@@ -1279,6 +1279,9 @@ Model evaluation and meta-estimators
     the parameter ``n_labels`` is renamed to ``n_groups``.
     :issue:`6660` by `Raghav RV`_.

+   - The :mod:`sklearn.linear_model.randomized_l1` is deprecated.
+     :issue: `8995` by `Andreas Mueller`_.


There should be a list of deprecated functions in 0.19 in the whatsnew.

ogrisel

LGTM as well once @amueller's comments are addressed.

Removed the deprecated module's code from examples Added warning removed from narrative documentation fixed flake8 Changed deprecation style to GMM Added tests and fixed flake8 Added removed code and updated whats_new Modified whatsnew

Sentient07 · 2017-06-30T11:47:31Z

Addressed comments, squashed into one commit

ogrisel · 2017-06-30T16:20:07Z

The CI errors are all cause by mldata.org 500 errors. Merging.

ogrisel · 2017-06-30T16:20:37Z

Thanks @Sentient07!

Sentient07 force-pushed the sprint#2 branch from 8a9b544 to 1584597 Compare June 7, 2017 22:40

Sentient07 force-pushed the sprint#2 branch from d60b51b to 158cda7 Compare June 8, 2017 13:43

ldirer mentioned this pull request Jun 10, 2017

[MRG] Add deprecation warning for iid in BaseSearchCV #9103

Closed

jnothman added the Blocker label Jun 14, 2017

jnothman added this to the 0.19 milestone Jun 14, 2017

jnothman changed the title ~~deprecated randomized_l1~~ [MRG] deprecated randomized_l1 Jun 14, 2017

Sentient07 force-pushed the sprint#2 branch from 158cda7 to 8fdc9b7 Compare June 14, 2017 23:37

amueller changed the title ~~[MRG] deprecated randomized_l1~~ [WIP] deprecated randomized_l1 Jun 19, 2017

Sentient07 force-pushed the sprint#2 branch from 7b08bf7 to b730484 Compare June 20, 2017 12:39

amueller reviewed Jun 20, 2017

View reviewed changes

Sentient07 force-pushed the sprint#2 branch from b730484 to 47f0c20 Compare June 20, 2017 21:07

jnothman reviewed Jun 20, 2017

View reviewed changes

Sentient07 force-pushed the sprint#2 branch from 47f0c20 to 2a067de Compare June 28, 2017 14:05

Sentient07 changed the title ~~[WIP] deprecated randomized_l1~~ [MRG] deprecated randomized_l1 Jun 28, 2017

amueller reviewed Jun 29, 2017

View reviewed changes

amueller changed the title ~~[MRG] deprecated randomized_l1~~ [MRG + 1] deprecated randomized_l1 Jun 29, 2017

ogrisel approved these changes Jun 29, 2017

View reviewed changes

deprecated randomized_l1

8343bf1

Removed the deprecated module's code from examples Added warning removed from narrative documentation fixed flake8 Changed deprecation style to GMM Added tests and fixed flake8 Added removed code and updated whats_new Modified whatsnew

Sentient07 force-pushed the sprint#2 branch from 2a067de to 8343bf1 Compare June 30, 2017 11:47

ogrisel merged commit c10c886 into scikit-learn:master Jun 30, 2017

massich pushed a commit to massich/scikit-learn that referenced this pull request Jul 13, 2017

Deprecate randomized_l1 (scikit-learn#9031)

11fffa4

balakmran mentioned this pull request Jul 20, 2017

Fix remaining sphinx errors #9417

Closed

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

Deprecate randomized_l1 (scikit-learn#9031)

1c8ec0b

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

Deprecate randomized_l1 (scikit-learn#9031)

0db8355

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

Deprecate randomized_l1 (scikit-learn#9031)

ccaa3bf

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

Deprecate randomized_l1 (scikit-learn#9031)

63c8ea3

AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

Deprecate randomized_l1 (scikit-learn#9031)

6382c6c

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

Deprecate randomized_l1 (scikit-learn#9031)

674b521

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

Deprecate randomized_l1 (scikit-learn#9031)

531f5c0

		@@ -58,6 +60,8 @@ def _resample_model(estimator_func, X, y, scaling=.5, n_resampling=200,
		return scores_


		@deprecated("The class BaseRandomizedLinearModel is deprecated in 0.19"

[MRG + 1] deprecated randomized_l1 #9031

[MRG + 1] deprecated randomized_l1 #9031

Conversation

Sentient07 commented Jun 7, 2017

Sentient07 commented Jun 7, 2017

agramfort commented Jun 7, 2017

jnothman commented Jun 7, 2017 via email

Sentient07 commented Jun 7, 2017

agramfort commented Jun 8, 2017

agramfort commented Jun 8, 2017

Sentient07 commented Jun 8, 2017

Sentient07 commented Jun 8, 2017

agramfort commented Jun 8, 2017 via email

Sentient07 commented Jun 8, 2017 • edited Loading

jnothman commented Jun 8, 2017 via email

jnothman commented Jun 14, 2017

Sentient07 commented Jun 14, 2017

jnothman commented Jun 14, 2017 via email

Sentient07 commented Jun 15, 2017

amueller commented Jun 19, 2017

Sentient07 commented Jun 19, 2017 via email

Sentient07 commented Jun 20, 2017

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Jun 20, 2017

Sentient07 commented Jun 20, 2017 • edited Loading

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massich commented Jun 28, 2017

Sentient07 commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

Sentient07 commented Jun 30, 2017

ogrisel commented Jun 30, 2017

ogrisel commented Jun 30, 2017

Sentient07 commented Jun 8, 2017 •

edited

Loading

Sentient07 commented Jun 20, 2017 •

edited

Loading