Repository: scikit-learn/scikit-learn
Pull request: [MRG + 1] BUG: Uses self.scoring for score function
#### Reference Issues/PRs
Fixes https://github.com/scikit-learn/scikit-learn/issues/10998.

#### What does this implement/fix? Explain your changes.
Adds a `score` function to `LogisticRegressionCV` that uses `self.scoring`.

Problem statement:
LogisticRegressionCV.score doesn't respect scoring, inconsistent with GridSearchCV
From what I can see, LogisticRegressionCV.score always computes accuracy, not the metric given by ``scoring``. That's inconsistent with the behavior of ``GridSearchCV``. How do other ``CV`` estimators do that? I assume the same?
That's pretty unfortunate and I think we should change it. We did the same backward incompatible change in GridSearchCV before with a warning.

Hints from issue discussion:
I feel this is a duplicate but can't find any other issues. #2709 is somewhat related.
Also related to #4668 though I think the issue here is more clear as the user provided a metric.
Looks like I complained about that in GridSearchCV in 2013 #1831
Just looked at the code for  ```LogisticRegressionCV.score```.  Can you give an example of what you are saying it is not very clear from the code? ```LogisticRegressionCv``` calls the ```_log_reg_scoring_path``` function which does compute the scores based on the given metric.
he is talking about the score method, which looks like it is inherited from
ClassifierMixin, and only measures accuracy.

@amueller, should we be fixing this in ClassifierMixin and altering
behaviour whenever a scoring parameter is present?

@jnothman hm that might be a good idea, though maybe slightly magic? It might change derived classes in user code. Though it might change it to the expected result... 
yes probably too magic and risky

On 1 May 2018 3:41 am, "Andreas Mueller" <notifications@github.com> wrote:

> @jnothman <https://github.com/jnothman> hm that might be a good idea,
> though maybe slightly magic? It might change derived classes in user code.
> Though it might change it to the expected result...
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/10998#issuecomment-385473544>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz627UZmthxgW76QfR90OcVkTwW0hUks5tt0zJgaJpZM4TcDTQ>
> .
>

@agramfort and @GaelVaroquaux might have opinions?

We could add a "common" check for everything having a scoring parameter though.
indeed it's a bug. It's because the score method from LogisticRegressionCV is derived from LogisticRegression. We need to add a score method in LogisticRegressionCV that is using self.scoring
I would like to take this up. If the method suggested @agramfort is the way to go I feel I can contribute to this.
Yes, and adding tests. But we also need to check that this is the same for other *CV estimators.
I'm not sure I'm posting this in the right place, but I came here via https://datascience.stackexchange.com/questions/17620/scoring-argument-in-scikit-learn-lassocv-lassolarscv-elasticnetcv because I expected to be able to pass a custom scoring function to `ElasticNetCV`, but couldn't find a way to do it. This issue seemed relevant.

Just wanted to give you a headsup, I'm not even sure it's at all possible to implement custom scoring for `ElasticNetCV`.

Commit message: DOC: Updates whats_new

Patch to review:
diff --git a/doc/whats_new/v0.20.rst b/doc/whats_new/v0.20.rst
index 0e206878862ae..55b4d0b59a63c 100644
--- a/doc/whats_new/v0.20.rst
+++ b/doc/whats_new/v0.20.rst
@@ -326,6 +326,11 @@ Classifiers and regressors
   returning incorrect probabilities in the case of binary outcomes.
   :issue:`9939` by :user:`Roger Westover <rwolst>`.
 
+- Fixed a bug in :class:`linear_model.LogisticRegressionCV` where the
+  ``score`` method always computes accuracy, not the metric given by
+  the ``scoring`` parameter.
+  :issue:`10998` by :user:`Thomas Fan <thomasjpfan>`.
+
 - Fixed a bug in :class:`linear_model.OrthogonalMatchingPursuit` that was
   broken when setting ``normalize=False``.
   :issue:`10071` by `Alexandre Gramfort`_.
diff --git a/sklearn/linear_model/logistic.py b/sklearn/linear_model/logistic.py
index 3e8a104d57d75..c2626fb1fa1c5 100644
--- a/sklearn/linear_model/logistic.py
+++ b/sklearn/linear_model/logistic.py
@@ -29,7 +29,8 @@
 from ..utils.fixes import logsumexp
 from ..utils.optimize import newton_cg
 from ..utils.validation import check_X_y
-from ..exceptions import NotFittedError, ConvergenceWarning
+from..exceptions import (NotFittedError, ConvergenceWarning,
+                         ChangedBehaviorWarning)
 from ..utils.multiclass import check_classification_targets
 from ..externals.joblib import Parallel, delayed
 from ..model_selection import check_cv
@@ -1789,3 +1790,37 @@ def fit(self, X, y, sample_weight=None):
 
         self.C_ = np.asarray(self.C_)
         return self
+
+    def score(self, X, y, sample_weight=None):
+        """Returns the score using the `scoring` option on the given
+        test data and labels.
+
+        Parameters
+        ----------
+        X : array-like, shape = (n_samples, n_features)
+            Test samples.
+
+        y : array-like, shape = (n_samples) or (n_samples, n_outputs)
+            True labels for X.
+
+        sample_weight : array-like, shape = [n_samples], optional
+            Sample weights.
+
+        Returns
+        -------
+        score : float
+            Score of self.predict(X) wrt. y.
+
+        """
+
+        if self.scoring is not None:
+            warnings.warn("The long-standing behavior to use the "
+                          "accuracy score has changed. The scoring "
+                          "parameter is now used. "
+                          "This warning will disappear in version 0.22.",
+                          ChangedBehaviorWarning)
+        scoring = self.scoring or 'accuracy'
+        if isinstance(scoring, six.string_types):
+            scoring = get_scorer(scoring)
+
+        return scoring(self, X, y, sample_weight=sample_weight)
diff --git a/sklearn/linear_model/tests/test_logistic.py b/sklearn/linear_model/tests/test_logistic.py
index a179c89e199a3..3c9d15b77b7d4 100644
--- a/sklearn/linear_model/tests/test_logistic.py
+++ b/sklearn/linear_model/tests/test_logistic.py
@@ -1,6 +1,9 @@
 import numpy as np
 import scipy.sparse as sp
 from scipy import linalg, optimize, sparse
+
+import pytest
+
 from sklearn.datasets import load_iris, make_classification
 from sklearn.metrics import log_loss
 from sklearn.model_selection import StratifiedKFold
@@ -19,6 +22,7 @@
 from sklearn.utils.testing import assert_warns_message
 
 from sklearn.exceptions import ConvergenceWarning
+from sklearn.exceptions import ChangedBehaviorWarning
 from sklearn.linear_model.logistic import (
     LogisticRegression,
     logistic_regression_path, LogisticRegressionCV,
@@ -89,6 +93,49 @@ def test_error():
         assert_raise_message(ValueError, msg, LR(max_iter="test").fit, X, Y1)
 
 
+def test_logistic_cv_mock_scorer():
+
+    class MockScorer(object):
+        def __init__(self):
+            self.calls = 0
+            self.scores = [0.1, 0.4, 0.8, 0.5]
+
+        def __call__(self, model, X, y, sample_weight=None):
+            score = self.scores[self.calls % len(self.scores)]
+            self.calls += 1
+            return score
+
+    mock_scorer = MockScorer()
+    Cs = [1, 2, 3, 4]
+    cv = 2
+
+    lr = LogisticRegressionCV(Cs=Cs, scoring=mock_scorer, cv=cv)
+    lr.fit(X, Y1)
+
+    # Cs[2] has the highest score (0.8) from MockScorer
+    assert lr.C_[0] == Cs[2]
+
+    # scorer called 8 times (cv*len(Cs))
+    assert mock_scorer.calls == cv * len(Cs)
+
+    # reset mock_scorer
+    mock_scorer.calls = 0
+    with pytest.warns(ChangedBehaviorWarning):
+        custom_score = lr.score(X, lr.predict(X))
+
+    assert custom_score == mock_scorer.scores[0]
+    assert mock_scorer.calls == 1
+
+
+def test_logistic_cv_score_does_not_warn_by_default():
+    lr = LogisticRegressionCV(cv=2)
+    lr.fit(X, Y1)
+
+    with pytest.warns(None) as record:
+        lr.score(X, lr.predict(X))
+    assert len(record) == 0
+
+
 def test_lr_liblinear_warning():
     n_samples, n_features = iris.data.shape
     target = iris.target_names[iris.target]


Code review comments:
diff_hunk: @@ -1789,3 +1790,37 @@ def fit(self, X, y, sample_weight=None):
 
         self.C_ = np.asarray(self.C_)
         return self
+
+    def score(self, X, y, sample_weight=None):
+        """Returns the score using the `scoring` option on the given
+        test data and labels.
+
+        Parameters
+        ----------
+        X : array-like, shape = (n_samples, n_features)
+            Test samples.
+
+        y : array-like, shape = (n_samples) or (n_samples, n_outputs)
path: sklearn/linear_model/logistic.py
text: @user1:
Do we support ``(n_samples, n_outputs)``? Seems that in ``fit``, we only support ``(n_samples,)``

@user2:
I agree. It should be `(n_samples,)`