[MRG+1] add docs that C can receive array in RandomizedLogisticRegression #6537

pianomania · 2016-03-14T06:36:35Z

fix #6525
closes #6536.

GaelVaroquaux · 2016-03-14T06:38:52Z

This is PR duplicates #6536, and I'll make the same comment on it.

amueller · 2016-10-27T17:34:19Z

Are you still working on this?

pianomania · 2016-10-29T01:47:05Z

No, but I can keep going if I have free time. What else should I do? Rebase it?

amueller · 2016-11-01T19:42:44Z

What @GaelVaroquaux said in the other PR: "You should indica 8000 te the behavior when C is an array: what are the different values of the array used for."

pianomania · 2016-11-25T03:00:02Z

@amueller I found I have written about the behavior when C is an array in this commit before. Are there some problems?

amueller · 2016-11-30T21:37:27Z

Oh sorry I somehow overlooked that. @GaelVaroquaux good?

amueller · 2016-11-30T21:40:20Z

sklearn/linear_model/randomized_l1.py

        The regularization parameter C in the LogisticRegression.
+        When C is an array, fit will take each regularization parameter in C 
+        one by one for LogisticRegression and store results for each one 
+        in all_scores_, where columns and rows represent corresponding 


can you please use double backticks around all_scores_? Not only is it a variable, the documentation generation breaks if something ends with an underscore and is not escaped.

codecov · 2017-02-17T08:42:22Z

Codecov Report

Merging #6537 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6537      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files         342      342              
  Lines       60801    60813      +12     
==========================================
+ Hits        57609    57621      +12     
  Misses       3192     3192

Impacted Files	Coverage Δ
sklearn/linear_model/tests/test_randomized_l1.py	`100% <100%> (ø)`	✅
sklearn/linear_model/randomized_l1.py	`94.76% <100%> (+0.06%)`	✅
sklearn/model_selection/tests/test_search.py	`99.31% <ø> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8694278...da90617. Read the comment docs.

GaelVaroquaux · 2017-02-17T09:22:23Z

LGTM. +1 for merge.

lesteve

There is a flake8 problem to fix. Other than that minor comments

lesteve · 2017-02-17T10:32:02Z

sklearn/linear_model/randomized_l1.py

@@ -381,8 +384,12 @@ class RandomizedLogisticRegression(BaseRandomizedLinearModel):

    Parameters
    ----------
-    C : float, optional, default=1
+    C : float or array of shape [n_reg_parameter], optional, default=1


array-like rather than array

lesteve · 2017-02-17T10:35:45Z

sklearn/linear_model/randomized_l1.py

@@ -354,6 +354,9 @@ def _randomized_logistic(X, y, weights, mask, C=1., verbose=False,
        X *= (1 - weights)

    C = np.atleast_1d(np.asarray(C, dtype=np.float64))
+    if C.ndim > 1:
+        raise ValueError("C should be 1-dim array, but got %d-dim array instead." % C.ndim)


1-dimensional or 1d rather than 1-dim. Same comment about %d-dim.

GaelVaroquaux · 2017-02-17T10:37:30Z

By the way: travis is failing. Could you rebase on master to see if this goes away.

lesteve · 2017-02-17T10:38:11Z

By the way: travis is failing. Could you rebase on master to see if this goes away.

That's a flake8 issue that needs fixing.

lesteve · 2017-02-17T11:35:20Z

sklearn/linear_model/randomized_l1.py

@@ -355,7 +355,8 @@ def _randomized_logistic(X, y, weights, mask, C=1., verbose=False,

    C = np.atleast_1d(np.asarray(C, dtype=np.float64))
    if C.ndim > 1:
-        raise ValueError("C should be 1-dim array, but got %d-dim array instead." % C.ndim)
+        raise ValueError("C should be 1-dim array, \


If you decided to go for 1-dimensional use it everywhere (and that's a s and not a t in dimensional), i.e. "should be a 1-dimensional array" and "but got a %d-dimensional array".

Also you don't need the backslash:

raise ValueError("beginning of the string " "that continues still here and with a format: {}".format("insert this")

sorry for that typo

lesteve · 2017-02-17T12:08:15Z

sklearn/linear_model/randomized_l1.py

@@ -354,6 +354,10 @@ def _randomized_logistic(X, y, weights, mask, C=1., verbose=False,
        X *= (1 - weights)

    C = np.atleast_1d(np.asarray(C, dtype=np.float64))
+    if C.ndim > 1:
+        raise ValueError("C should be 1-dimensional array, "
+            "but got %d-dimensional array instead." % C.ndim)


I forgot: it is always good to show the parameter that you got because it gives an additional hint to the user what he needs to change.

So something like this (add the "a" that were missed from my previous comment):

raise ValueError("C should be a 1-dimensional array-like, " "but got a {}-dimensional array-like instead: {}".format(C.ndim, C))

It would be great to add a test for this error message by the way ...

OK, I will add a test!

lesteve · 2017-02-17T15:20:51Z

sklearn/linear_model/randomized_l1.py

@@ -354,6 +354,11 @@ def _randomized_logistic(X, y, weights, mask, C=1., verbose=False,
        X *= (1 - weights)

    C = np.atleast_1d(np.asarray(C, dtype=np.float64))
+    if C.ndim > 1:
+        raise ValueError("C should be 1-dimensional array, "
+                         "but got a %d-dimensional array instead: %d."


This is going to give an error because "%d" is not a valid format for an array ... this is why we need a test.

Please use .format rather than % (less chance of making silly mistakes like this). Also please use "array-like" and not "array" as mentioned in one of my previous comment.

agramfort · 2017-02-18T06:38:16Z

thx @pianomania

…sion (scikit-learn#6537) * doc: state that parameter C can receive an array * add more details in doc, and check the dim of C * doc: state that parameter C can receive an array * add more details in doc, and check the dim of C * a little modification * minor modifications and make line length less than 79 characters * remove the backslash and correct typos * meet PEP8's E128 requirement * use .format and add a test

doc: state that parameter C can receive an array

4db495b

add more details in doc, and check the dim of C

3535c00

amueller reviewed Nov 30, 2016

View reviewed changes

pianomania added 3 commits February 17, 2017 15:55

doc: state that parameter C can receive an array

8c23d91

add more details in doc, and check the dim of C

3f6ea47

a little modification

7e44df4

pianomania force-pushed the RandomizedLogisticRegression_doc branch from 3535c00 to 7e44df4 Compare February 17, 2017 08:22

add double backticks

d4c7714

GaelVaroquaux changed the title ~~[MRG] add docs that C can receive array in RandomizedLogisticRegression~~ [MRG+1] add docs that C can receive array in RandomizedLogisticRegression Feb 17, 2017

lesteve reviewed Feb 17, 2017

View reviewed changes

minor modifications and make line length less than 79 characters

96deb31

lesteve reviewed Feb 17, 2017

View reviewed changes

remove the backslash and correct typos

19a4dc6

lesteve reviewed Feb 17, 2017

View reviewed changes

meet PEP8's E128 requirement

dc85f4e

lesteve reviewed Feb 17, 2017

View reviewed changes

use .format and add a test

da90617

agramfort merged commit f952e43 into scikit-learn:master Feb 18, 2017

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] add docs that C can receive array in RandomizedLogisticRegression #6537

[MRG+1] add docs that C can receive array in RandomizedLogisticRegression #6537

[MRG+1] add docs that C can receive array in RandomizedLogisticRegression #6537

[MRG+1] add docs that C can receive array in RandomizedLogisticRegression #6537

Conversation

Choose a reason for hiding this comment

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment