[MRG+1] DOC adding a warning on the relation between C and alpha #7860

dalmia · 2016-11-12T02:15:38Z

Reference Issue

What does this implement/fix? Explain your changes.

Added the note on varying equivalence between C and alpha depending on the estimator. Also, made a note in the linear models module warning about this dependence.

dalmia · 2016-11-16T03:56:30Z

@jnothman Could you please help me understand why circleci is giving an error and maybe provide a review of my changes?
Thank you.

jnothman · 2016-11-16T04:35:51Z

I've requested a rebuild. Let's see if that's enough.

jnothman · 2016-11-16T08:08:14Z

All green from the CI's perspective. Is this still WIP? WIP means you're not really ready for review yet.

dalmia · 2016-11-16T09:42:17Z

Sorry was stuck in class! Ready for review I suppose.

amueller · 2016-11-16T23:32:27Z

doc/modules/linear_model.rst

@@ -266,6 +266,11 @@ They also tend to break when the problem is badly conditioned

  * :ref:`sphx_glr_auto_examples_linear_model_plot_lasso_model_selection.py`

+.. note:: **Comparison with the regularization parameter of SVM**


wouldn't it be better to mention that directly after alpha is introduces? Also, there are only two possiblities, right? alpha = 1/C or alpha = n_samples / C?

I think @amueller is saying that the current description here is not specific enough, or not very helpful to the reader.

Thanks for clearing it up. Yes, I should add that only 2 variances of C exist (with respect to alpha). As per the positioning, I felt that since this could be critical to getting a correct result, it should be mentioned separately to draw more attention as it's importance could really be overlooked when mentioned under the same heading .
Also, I felt it was better that readers first understand what alpha does, so that they can make sense of what we are trying to add. I also had a suggestion that we link the issue there because at first I didn't know about this discontinuity but reading the comments on the thread made it clear for me, which could easily be the case with any new contributor.

Added the change mentioning the two values of C, but am skeptical about mentioning it at the top. Please let me know what you feel.

TomDLT · 2016-11-21T10:39:55Z

doc/modules/svm.rst

+    equivalence between the amount of regularization of two models depends on
+    the exact objective function optimized by the model. For example, when the
+    estimator used is ``Ridge Regression``, the relation between both is given as
+    :math:`C = \frac{n\_samples}{alpha}`.


It should be \frac{1}{alpha}

TomDLT · 2016-11-21T10:42:33Z

doc/modules/svm.rst

+    regularization parameter, most other estimators use ``alpha``. The exact
+    equivalence between the amount of regularization of two models depends on
+    the exact objective function optimized by the model. For example, when the
+    estimator used is ``Ridge Regression``, the relation between both is given as


estimator used is :class:`Ridge` regression, the relation between both is given as

"between both" -> "between them"

Thanks for the review, making the change.

jnothman

otherwise LGTM

jnothman · 2016-11-23T11:26:56Z

doc/modules/svm.rst

+    regularization parameter, most other estimators use ``alpha``. The exact
+    equivalence between the amount of regularization of two models depends on
+    the exact objective function optimized by the model. For example, when the
+    estimator used is :class:`Ridge` regression, the relation between them is given as


you need to qualify the reference to ridge with linear_model. or maybe even sklearn.linear_model.

dalmia · 2016-11-23T15:41:39Z

@jnothman Please have a look now.

jnothman · 2016-11-23T19:30:07Z

doc/modules/svm.rst

+    regularization parameter, most other estimators use ``alpha``. The exact
+    equivalence between the amount of regularization of two models depends on
+    the exact objective function optimized by the model. For example, when the
+    estimator used is :class:`sklearn.linear_model.Ridge` regression, the relation between them is given as


would look better as

:class:`sklearn.linear_model.Ridge <ridge>`

but the link works at least

Actually, I was referencing how GridSearchCV is being. I thought it would be good to follow the common pattern of how the links are being inserted.

Proper choice of ``C`` and ``gamma`` is critical to the SVM's performance. One is advised to use :class:`sklearn.model_selection.GridSearchCV` with ``C`` and ``gamma`` spaced exponentially far apart to choose good values.

Yes, except that here "used is Ridge regression" is a syntactic use of "ridge" not just a mention of the estimator.

Thanks for clearing it up! I'll make the change then.

jnothman · 2016-11-24T03:55:51Z

doc/modules/svm.rst

+    regularization parameter, most other estimators use ``alpha``. The exact
+    equivalence between the amount of regularization of two models depends on
+    the exact objective function optimized by the model. For example, when the
+    estimator used is :class:`sklearn.linear_model.Ridge <ridge>` regression, the relation between them is given as


mind your line length, please

dalmia · 2016-11-24T06:34:41Z

I was looking to check if the link's working but for some reason, CircleCI is not showing the built artifact.

dalmia · 2016-11-26T13:55:41Z

@jnothman Please let me know if there is anything I need to change. Thanks.

jnothman · 2016-11-28T01:46:22Z

I've marked this as LGTM, but - although sometimes one approval suffices for simple documentation changes - I'd appreciate the review of another core dev.

TomDLT · 2016-11-29T19:13:24Z

This looks good to me

…kit-learn#7860) * DOC adding a warning on the relation between C and alpha * DOC removing extra character * DOC: changes to the relation described * DOC fixing typo * DOC fixing typo * DOC fixing link to Ridge * DOC link enhancement * DOC fixing line length

dalmia added 2 commits November 12, 2016 07:36

DOC adding a warning on the relation between C and alpha

3121601

DOC removing extra character

a4c7abb

dalmia changed the title ~~[WIP] DOC adding a warning on the relation between C and alpha~~ [MRG] DOC adding a warning on the relation between C and alpha Nov 16, 2016

amueller reviewed Nov 16, 2016

View reviewed changes

DOC: changes to the relation described

a69668c

TomDLT reviewed Nov 21, 2016

View reviewed changes

dalmia added 2 commits November 23, 2016 04:02

DOC fixing typo

79de888

DOC fixing typo

b1ef169

jnothman requested changes Nov 23, 2016

View reviewed changes

DOC fixing link to Ridge

882d4bf

jnothman approved these changes Nov 23, 2016

View reviewed changes

jnothman changed the title ~~[MRG] DOC adding a warning on the relation between C and alpha~~ [MRG+1] DOC adding a warning on the relation between C and alpha Nov 23, 2016

DOC link enhancement

d942073

jnothman reviewed Nov 24, 2016

View reviewed changes

DOC fixing line length

03c2aa3

jnothman added Documentation Waiting for Reviewer labels Nov 28, 2016

TomDLT merged commit 8793ec9 into scikit-learn:master Nov 29, 2016

dalmia deleted the 6919 branch November 29, 2016 20:39

		@@ -266,6 +266,11 @@ They also tend to break when the problem is badly conditioned

		* :ref:`sphx_glr_auto_examples_linear_model_plot_lasso_model_selection.py`

		.. note:: Comparison with the regularization parameter of SVM

Uh oh!

[MRG+1] DOC adding a warning on the relation between C and alpha #7860

[MRG+1] DOC adding a warning on the relation between C and alpha #7860

Uh oh!

Conversation

dalmia commented Nov 12, 2016

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

dalmia commented Nov 16, 2016

Uh oh!

jnothman commented Nov 16, 2016

Uh oh!

jnothman commented Nov 16, 2016

Uh oh!

dalmia commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 23, 2016

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!

dalmia commented Nov 24, 2016

Uh oh!

dalmia commented Nov 26, 2016

Uh oh!

jnothman commented Nov 28, 2016

Uh oh!

TomDLT commented Nov 29, 2016

Uh oh!

Uh oh!

dalmia Nov 18, 2016 •

edited

Loading

TomDLT Nov 21, 2016 •

edited

Loading

TomDLT Nov 21, 2016 •

edited

Loading