[MRG+1] Fix broken `L1` deprecations and svm scale C example. by raghavrv · Pull Request #4261 · scikit-learn/scikit-learn

raghavrv · 2015-02-17T14:02:05Z

@ogrisel Please take a look...

Fix Broken plot_svm_scale_c.py example #4260
loss, penalty --> penalty, loss in error message
Add tests to check for error message when invalid loss and penalty are passed.
Deprecate and support L1 ( upper case )

GaelVaroquaux · 2015-02-17T14:24:35Z

Though we explicitly specify that losses must be 'l1' and not 'L1', should we
support 'L1' ?

In my experience, it is a bad idea to introduce too much flexibility:
think of someone storing results in a dictionnary, where 'penalty' is a
key of the dictionnary. If we are flexible, that person will just hit a
bug in the dictionnary part, and not in our code.

raghavrv · 2015-02-17T14:43:15Z

Ah makes sense! thanks for clarifying :)

ogrisel · 2015-02-17T14:46:10Z

examples/svm/plot_svm_scale_c.py

It should be squared_hinge, instead of ssquared_hinge, no?

Ah! I did this to test the first commit and forgot to change it back ;)

ogrisel · 2015-02-17T14:54:55Z

This example use to work in scikit-learn 0.15.2: http://scikit-learn.org/stable/auto_examples/svm/plot_svm_scale_c.html

This means that this change introduced without preserving backward compatibility. We need to add support for the old notation while raising informative deprecation warning messages such as:

loss='L2' has been deprecated and renamed to loss='squared_hingeloss' in 0.16. Support for the old parameter will be dropped in 0.18

ogrisel · 2015-02-17T14:57:11Z

Furthermore this API change should be documented in whats new:

http://scikit-learn.org/dev/whats_new.html#api-changes-summary

raghavrv · 2015-02-17T14:58:45Z

'l2' is still supported... and does display that deprecation warning!! ( the problem was with the 'L2' ( upper case ) ).
I had change it to 'squared_hinge' assuming examples don't need deprecation?

Also see #3933 ...

ogrisel · 2015-02-17T15:00:16Z

This change of behavior might have been introduced by the integration of the new LinearSVR class that uses some common liblinear backend (this has to be checked).

If that the case we need to check that the LogisticRegression class is not similarly impacted. In that case we will to setup backward compat with deprecation warning there as well (only for the penalty as the loss is fixed for that model).

ogrisel · 2015-02-17T15:02:10Z

'l2' is still supported... and does display that deprecation warning!! ( the problem was with the 'L2' ( upper case ) ).

L2 with uppercase needs to be supported (with deprecation warning) as it was part of the public API in the last stable release 0.15.2.

raghavrv · 2015-02-17T15:03:21Z

L2 with uppercase needs to be supported

Oh! okay! will find out what caused the change and deprecate it properly :)

ogrisel · 2015-02-17T15:04:28Z

By reading the diff of https://github.com/scikit-learn/scikit-learn/pull/3933/files , the code used to be case insensitive, which is no longer the case. +1 for deprecating explicitly the upper case notation for the l1 and l2 penalties and losses.

amueller · 2015-02-17T19:25:32Z

LGTM apart from @ogrisel comment on tighter testing and the typo that gives the travis error ;)

ogrisel · 2015-02-18T10:30:10Z

sklearn/svm/classes.py

Please also add backward compat for penalty in ('L1', 'L2') (uppercase) that maps to the l1 or l2 (lowercase) with a similar deprecation warning.

And add deprecation warning tests for those. Please git grep "assert_warns_message(DeprecationWarning" to find examples on how to write such tests.

this is done in this line...

woops I clicked on the edit button instead of the reply button and deleted part of your reply by mistake... Sorry for that.

Anyway I wanted to also let you know that you can drop the nested parens in the above call to warnings.warn.

Oh! thats good to know! thanks... I keep doing that a lot! ;)

ogrisel · 2015-02-18T12:28:50Z

sklearn/svm/base.py

We need to provide explicit and informative warning and error messages that recall the values of both the observed and expected values. In this case:

if loss in ('L1', 'L2'): warnings.warn("loss='%s' has been deprecated in favor of loss='%s' as of 0.16. " "Backward compatibility for the uppercase notation will be removed" " in 0.18." % (loss, loss.lower()))

and a similar message for penalty.

We should not assume that the user has the source code at hand with the precise line where this class is instantiated when executing a program that causes this message to be raised. Putting explicit parameter values in the message is likely to help him / her find the source of the problem much more quickly.

done ;)

This is not addressed in the last commit currently visible on github (635b933). Travis is still broken at this point.

Ah :/ Sorry!! I'll fix this soon... ( I must have forgotten to push I guess )

raghavrv · 2015-02-24T20:00:22Z

@ogrisel could you please take a look at this now? Have added the tests and done deprecations as follows -

loss 'L1' / 'l1' deprecated with warning to 'hinge' for LinearSVC or 'epsilon_insensitive' for LinearSVR inside their respective __init__s. ( like wise for 'L2'/'l2' )
Uppercase notations for loss ( when loss is not 'l1'/'l2' ) and penalty are deprecated inside _fit_liblinear and warning will be raised when they are fit ( to support LinearSVC / LinearSVR / LogisticRegression and LogisticRegressionCV )

amueller · 2015-02-24T22:37:10Z

You can't deprecate inside an __init__ as the attribute might be set using grid search. So you need to deprecate in fit.

amueller · 2015-02-24T22:37:45Z

Tests still failing btw.

raghavrv · 2015-02-25T12:30:06Z

done! please take a look now :)

raghavrv · 2015-02-25T12:30:44Z

sklearn/svm/base.py

Perhaps the old one was more readable?

You mean the formatting of the nested dict? I agree I preferred the old formatting.

Yea :) thanks will revert that!

ogrisel · 2015-02-25T13:33:27Z

Apart from this last cosmetic comment, +1 on my side.

ogrisel · 2015-02-25T15:23:08Z

Ok merging this!

[MRG+1] Broken svm scale C example

ogrisel · 2015-02-25T15:23:20Z

Thanks again @ragv!

ogrisel reviewed Feb 17, 2015
View reviewed changes

raghavrv force-pushed the svm_scale_c_plot branch from 1ae37dd to 3a8eae0 Compare February 17, 2015 14:54

raghavrv force-pushed the svm_scale_c_plot branch from 776af2d to a28dba9 Compare February 17, 2015 23:02

ogrisel reviewed Feb 18, 2015
View reviewed changes

ogrisel changed the title ~~[MRG] Svm scale c plot~~ [MRG] Broken svm scale C example Feb 18, 2015

ogrisel reviewed Feb 18, 2015
View reviewed changes

amueller added this to the 0.16 milestone Feb 23, 2015

raghavrv force-pushed the svm_scale_c_plot branch 2 times, most recently from 01143b7 to a878fac Compare February 24, 2015 19:49

raghavrv force-pushed the svm_scale_c_plot branch from a878fac to 3429357 Compare February 25, 2015 10:02

raghavrv added 3 commits February 25, 2015 15:34

FIX loss, penalty --> penalty, loss

c1c8cc1

EXAMPLE/FIX l2 --> squared_hinge; L1/L2 --> l1/l2

6d23a58

FIX/MAINT Deprecate and support upper case values for loss/penalty

9df01a8

raghavrv force-pushed the svm_scale_c_plot branch 2 times, most recently from 8e055a8 to 802e5b4 Compare February 25, 2015 10:59

raghavrv reviewed Feb 25, 2015
View reviewed changes

raghavrv added 3 commits February 25, 2015 19:12

COSMIT Flake8 fixes for svm/base.py

50876ec

FIX Make LinearSVC/LinearSVR support the deprecated l1/L1, l2/L2

354c35c

TST test deprecation warning and support for uppercase

6d222fd

raghavrv force-pushed the svm_scale_c_plot branch from 802e5b4 to 6d222fd Compare February 25, 2015 13:42

raghavrv changed the title ~~[MRG] Broken svm scale C example~~ [MRG+1] Broken svm scale C example Feb 25, 2015

ogrisel added a commit that referenced this pull request Feb 25, 2015

Merge pull request #4261 from ragv/svm_scale_c_plot

a926626

[MRG+1] Broken svm scale C example

ogrisel merged commit a926626 into scikit-learn:master Feb 25, 2015

raghavrv deleted the svm_scale_c_plot branch March 18, 2015 22:57

raghavrv changed the title ~~[MRG+1] Broken svm scale C example~~ [MRG+1] Fix broken L1 deprecations and svm scale C example. Mar 23, 2015

This was referenced Oct 19, 2015

Remove deprecated stuff that will no longer be supported by 0.18 #5434

Closed

[MRG + 1] Remove deprecated stuff from SVM #5469

Merged

Uh oh!

Conversation

raghavrv commented Feb 17, 2015

Uh oh!

GaelVaroquaux commented Feb 17, 2015

Uh oh!

raghavrv commented Feb 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

raghavrv commented Feb 17, 2015

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

raghavrv commented Feb 17, 2015

Uh oh!

ogrisel commented Feb 17, 2015

Uh oh!

amueller commented Feb 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv commented Feb 24, 2015

Uh oh!

amueller commented Feb 24, 2015

Uh oh!

amueller commented Feb 24, 2015

Uh oh!

raghavrv commented Feb 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 25, 2015

Uh oh!

ogrisel commented Feb 25, 2015

Uh oh!

ogrisel commented Feb 25, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants