Repository: scikit-learn/scikit-learn
Pull request: MNT Deprecate coef_ and intercept_ in naive bayes estimators
#### Reference Issues/PRs

Closes #2237.

#### What does this implement/fix? Explain your changes.

This PR deprecates the attributes `coef_` and `intercept_` in `sklearn.naive_bayes.MultinomialNB`, `sklearn.naive_bayes.BernoulliNB` and `sklearn.naive_bayes.CategoricalNB`.

Problem statement:
Naive Bayes has the wrong coef_ and intercept_
The coef_ and intercept_ on MultinomialNB don't behave like other linear models because they contain probabilities even in the binary case. They should contain log-odds ratios.

The ones in BernoulliNB (not documented) are even worse: they're not the coef and intercept used to make the actual prediction (see `BernoulliNB._joint_log_likelihood`) so they can't be used for model inspection. This is a hairy issue in terms of backward compat.

Hints from issue discussion:
Moved to 0.14 for now, though I'm not sure if I can resolve it properly before then.

fair enough :)

I am retagging for 0.15. @larsmans, if you think that you can do this before, will free, but it doesn't seem very high priority to me.

Tagging to 1.0

I think we should fix this and I am happy to dispatch a student for it.
We are changing the meaning of `coef_` here. I'm not sure if we ever had a similar issue where we changed the meaning of an attribute. Maybe in the scalers recently.

I see two options:

a) raise a FutureWarning now if the user directly accesses `coef_`. The user has to explicitly silence the warning if they know what they are doing. Then we can do the actual change in two versions (if only we had done this when the issue was raised lol).
Downside: the user can't fix their code now. They will get a warning, notice the warning, then probably silence it, and their code will break when the change happens.

b) add a new parameter `old_coef=None`, let the user specify "True" for the old behavior, "False" for the new behavior, raise a warning if the default is given that the default will change from True to False. After the default changed, two more versions to remove the parameter again.
Downside: 4 version cycle.

I think b) is better. Maybe with another parameter name?

if coef contains probabilities maybe call it coef_proba_ and add a
coef_logodds_
then with a param you can with a @property have coef being one or the
other. Make this param None and raise a warning in that case

makes sense?

sounds ok

Hi SciKit team! I am interested in solving this long-lost issue so these models can be inspected. This is my first time looking at SciKit internals, so I want to check that I'm reading this right:

- The initial PR here ( https://github.com/scikit-learn/scikit-learn/pull/2250 ), which uses ```feature_log_prob```,  was doing the right math. They vary the response a little bit depending on which class of Bayesian we are talking about
- as described in this thread, a PR should add parameter ```old_coef``` and warn if it is not set
- as described in this thread, ```coef_``` will (if directed) call ```coef_logodds_``` and return the new values
- update existing tests and docs, add a new test and docs

First pass at this (not functioning, just showing where the code goes): https://github.com/mapmeld/scikit-learn/commit/f28dc5270ea02778426d45ceac6b1502be10dace

Please let me know if there is a Slack/IRC channel or similar to talk through how this would work
I think we should just deprecate coef_ here as it means something different to is use elsewhere.
+1

Naive Bayes models are not linear models (without extra non classical
constraints).

Commit message: FIX More deprecations in tests

Patch to review:
diff --git a/sklearn/naive_bayes.py b/sklearn/naive_bayes.py
index 11246203a8b94..0ba25f68054e8 100644
--- a/sklearn/naive_bayes.py
+++ b/sklearn/naive_bayes.py
@@ -27,13 +27,14 @@
 from .preprocessing import binarize
 from .preprocessing import LabelBinarizer
 from .preprocessing import label_binarize
-from .utils import check_X_y, check_array
+from .utils import check_X_y, check_array, deprecated
 from .utils.extmath import safe_sparse_dot
 from .utils.multiclass import _check_partial_fit_first_call
 from .utils.validation import check_is_fitted, check_non_negative, column_or_1d
 from .utils.validation import _check_sample_weight
 from .utils.validation import _deprecate_positional_args
 
+
 __all__ = ['BernoulliNB', 'GaussianNB', 'MultinomialNB', 'ComplementNB',
            'CategoricalNB']
 
@@ -645,19 +646,22 @@ def _init_counters(self, n_effective_classes, n_features):
         self.feature_count_ = np.zeros((n_effective_classes, n_features),
                                        dtype=np.float64)
 
-    # XXX The following is a stopgap measure; we need to set the dimensions
-    # of class_log_prior_ and feature_log_prob_ correctly.
-    def _get_coef(self):
+    # mypy error: Decorated property not supported
+    @deprecated("Attribute coef_ was deprecated in "  # type: ignore
+                "version 0.24 and will be removed in 0.26.")
+    @property
+    def coef_(self):
         return (self.feature_log_prob_[1:]
                 if len(self.classes_) == 2 else self.feature_log_prob_)
 
-    def _get_intercept(self):
+    # mypy error: Decorated property not supported
+    @deprecated("Attribute intercept_ was deprecated in "  # type: ignore
+                "version 0.24 and will be removed in 0.26.")
+    @property
+    def intercept_(self):
         return (self.class_log_prior_[1:]
                 if len(self.classes_) == 2 else self.class_log_prior_)
 
-    coef_ = property(_get_coef)
-    intercept_ = property(_get_intercept)
-
     def _more_tags(self):
         return {'poor_score': True}
 
@@ -703,6 +707,9 @@ class MultinomialNB(_BaseDiscreteNB):
         Mirrors ``feature_log_prob_`` for interpreting MultinomialNB
         as a linear model.
 
+        .. deprecated:: 0.24
+            ``coef_`` is deprecated in 0.24 and will be removed in 0.26.
+
     feature_count_ : ndarray of shape (n_classes, n_features)
         Number of samples encountered for each (class, feature)
         during fitting. This value is weighted by the sample weight when
@@ -716,6 +723,9 @@ class MultinomialNB(_BaseDiscreteNB):
         Mirrors ``class_log_prior_`` for interpreting MultinomialNB
         as a linear model.
 
+        .. deprecated:: 0.24
+            ``intercept_`` is deprecated in 0.24 and will be removed in 0.26.
+
     n_features_ : int
         Number of features of each sample.
 
diff --git a/sklearn/tests/test_docstring_parameters.py b/sklearn/tests/test_docstring_parameters.py
index 94112a30469fe..36d60f2dd8c9c 100644
--- a/sklearn/tests/test_docstring_parameters.py
+++ b/sklearn/tests/test_docstring_parameters.py
@@ -216,7 +216,8 @@ def test_fit_docstring_attributes(name, Estimator):
     else:
         est.fit(X, y)
 
-    skipped_attributes = {'n_features_in_'}
+    # TODO: Remove "coef_" and "intercept_" in version 0.26
+    skipped_attributes = {'n_features_in_', 'coef_', 'intercept_'}
 
     for attr in attributes:
         if attr.name in skipped_attributes:
diff --git a/sklearn/tests/test_naive_bayes.py b/sklearn/tests/test_naive_bayes.py
index 3f15e548dde62..1121e153cd28c 100644
--- a/sklearn/tests/test_naive_bayes.py
+++ b/sklearn/tests/test_naive_bayes.py
@@ -193,6 +193,17 @@ def test_gnb_naive_bayes_scale_invariance():
     assert_array_equal(labels[1], labels[2])
 
 
+@pytest.mark.parametrize("cls", [MultinomialNB, BernoulliNB, CategoricalNB])
+def test_discretenb_deprecated_coef_intercept(cls):
+    est = cls().fit(X2, y2)
+
+    msg = "Attribute {} was deprecated"
+
+    for att in ["coef_", "intercept_"]:
+        with pytest.warns(FutureWarning, match=msg.format(att)):
+            getattr(est, att)
+
+
 @pytest.mark.parametrize("cls", [MultinomialNB, BernoulliNB, CategoricalNB])
 def test_discretenb_prior(cls):
     # Test whether class priors are properly set.
@@ -335,7 +346,6 @@ def test_discretenb_predict_proba():
         assert_almost_equal(np.sum(clf.predict_proba([X[1]])), 1)
         assert_almost_equal(np.sum(clf.predict_proba([X[-1]])), 1)
         assert_almost_equal(np.sum(np.exp(clf.class_log_prior_)), 1)
-        assert_almost_equal(np.sum(np.exp(clf.intercept_)), 1)
 
 
 @pytest.mark.parametrize('cls', [BernoulliNB, MultinomialNB, CategoricalNB])
@@ -409,19 +419,6 @@ def test_discretenb_sample_weight_multiclass(cls):
     assert_array_equal(clf.predict(X), [0, 1, 1, 2])
 
 
-@pytest.mark.parametrize('cls', [BernoulliNB, MultinomialNB])
-def test_discretenb_coef_intercept_shape(cls):
-    # coef_ and intercept_ should have shapes as in other linear models.
-    # Non-regression test for issue #2127.
-    X = [[1, 0, 0], [1, 1, 1]]
-    y = [1, 2]  # binary classification
-    clf = cls()
-
-    clf.fit(X, y)
-    assert clf.coef_.shape == (1, 3)
-    assert clf.intercept_.shape == (1,)
-
-
 @pytest.mark.parametrize('kind', ('dense', 'sparse'))
 def test_mnnb(kind):
     # Test Multinomial Naive Bayes classification.
@@ -501,17 +498,6 @@ def test_mnb_prior_unobserved_targets():
     assert clf.predict([[1, 1]]) == 2
 
 
-def test_mnb_sample_weight():
-    clf = MultinomialNB()
-    clf.fit([[1, 2], [1, 2], [1, 0]],
-            [0, 0, 1],
-            sample_weight=[1, 1, 4])
-    assert_array_equal(clf.predict([[1, 0]]), [1])
-    positive_prior = np.exp(clf.intercept_[0])
-    assert_array_almost_equal([1 - positive_prior, positive_prior],
-                              [1 / 3., 2 / 3.])
-
-
 def test_bnb():
     # Tests that BernoulliNB when alpha=1.0 gives the same values as
     # those given for the toy example in Manning, Raghavan, and


Code review comments:
diff_hunk: @@ -216,7 +216,8 @@ def test_fit_docstring_attributes(name, Estimator):
     else:
         est.fit(X, y)
 
-    skipped_attributes = {'n_features_in_'}
+    # TODO: Remove "coef_" and "intercept_" in version 0.26
+    skipped_attributes = {'n_features_in_', 'coef_', 'intercept_'}
path: sklearn/tests/test_docstring_parameters.py
text: @user1:
We should not skip these. We need something similar to:

https://github.com/scikit-learn/scikit-learn/blob/8cce5bfcf0f001d91deca145f878f474a0b8e894/sklearn/multiclass.py#L446-L457

make sure `hasattr` fails.

@author:
I have made a few of changes to check that `FutureWarning` is raised for deprecated attributes when testing the docstring.

WDYT about these minor changes?