Repository: scikit-learn/scikit-learn
Issue category: Feature Request

Issue:
[WIP] : Added assert_consistent_docs() and related tests

Fixes #9388 

Added a function to check for consistency between docstring of objects.

In this approach there is a python dictionary for each of Parameters, Attributes and Returns. Indexed by the name of the parameter/attribute/return with the value being a Python dictionary containing its type definition and description. The function checks for each object if the docstring is identical (excluding whitespaces) for a given parameter/attribute/return in the main dictionary. If a new parameter/attribute/return are found they are added in the main dictionary.


Hints:
Do we expect this to be reasonable for everything, or only for parts of the library? something like ``random_state`` might or might not be good to document consistently - saying what the randomness is might be good. Also, we have parameters like ``alpha`` that can mean basically anything
Or is the idea to call this with very small families, like "only linear models" or something like that?
numpydoc is not found? (are we not using numpydoc on master... I lost track of that)
can you choose a few related objects from one module and add tests for
their parameters as an example?

In `sklearn/tests/test_docstring_parameters.py` we skip the test if numpydoc is not found. We should do so here also. Only one of our Travis runs installs numpydoc for testing.
Yes, Andy, the intention is to use this for families of objects, e.g. `precision_score`, `recall_score`, `f1_score`, etc..
The doctest for the example is failing as I have used `try` and `except` for `import numpydoc` in the unit test and not in the function itself. What should I do?
do the import locally in the function is one easy solution

The doctest is still failing with
`UNEXPECTED EXCEPTION: SkipTest('numpydoc is required to test the docstrings, as well as python version >= 3.5',)`
Should we skip the doctest?
I think just skip, or remove, the doctest.

On 17 December 2017 at 16:11, Aman Pratik <notifications@github.com> wrote:

> The doctest is still failing with
> UNEXPECTED EXCEPTION: SkipTest('numpydoc is required to test the
> docstrings, as well as python version >= 3.5',)
> Should we skip the doctest?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/pull/10323#issuecomment-352232808>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz61TOYUDWAaKm6cvUFC7AjYZ3ggi2ks5tBKJngaJpZM4RCetf>
> .
>

I will add more tests to improve the coverage shortly.

After seeing the coverage report I feel because of `numpydoc` not being available many tests are not running, hence the bad coverage.
What do you say @jnothman  @amueller ?
@jnothman  What are we supposed to do finally about the include and exclude concept?
As I believe,
1. default for include can be set to '*'.
2. We can keep it mandatory to set either include or exclude only.
Let's get rid of '*' and replace it with True:
* Include=True by default
* Include=False disables that section
* Include=collection tests only named elements
* Exclude=None (equivalent to ()) by default
* Exclude otherwise can only be set if include is True

Yes, make a helper to only include a test when docstring testing is enabled
@jnothman I have made the changes and added tests. Need your opinion on the tests.
I have added validation test for include_ and exclude_ , but I need your opinion on it. Also, I am not sure on how to test the corner cases. Few examples might help.
I have tried to change the tests accordingly. I have used `assert_raise_message` instead of `assert_raises_regex` since I was having difficulty with regular expressions.
I will have to work on the edge cases. I get confused sometimes on which tests would be reasonable and which wont.
it's a hard balance to get right, and you get better with practice. writing
tests before implementation, and extending them during implementation helps
think about how to assert desired functionality.

Good tests should IMO look a bit like a proof by induction. You test the
base case, and then assume everything like what's been tested so far works,
upon which you extend with variants (different parameters etc)

Well, the code would show an error on the very first inconsistency it finds. The error message would be enough to locate the exact place of inconsistency i.e. the error message shows the Parameter name, name of the concerned objects and error type(type definition or description).
@glemaitre @scikit-learn/documentation-team what do we think of this now? There are a few usecases, worth continuing this work?
I guess this PR was already looking in good shape, so the extra step may be worth it.
+1, I think this is useful
@lucyleeow @ArturoAmorQ would you have bandwidth to push this forward? pretty please? 😁 
I'm happy to work on this 😄 
Please feel free to ping me as well if you need reviews :)
Yep this looks like a step ahead for consistency and having the right docstring. 

Patch:
diff --git a/sklearn/metrics/_classification.py b/sklearn/metrics/_classification.py
index 0175966a21b74..b69855633a730 100644
--- a/sklearn/metrics/_classification.py
+++ b/sklearn/metrics/_classification.py
@@ -1727,14 +1727,18 @@ def precision_recall_fscore_support(
         "assigned" 0 samples. For multilabel targets, labels are column indices.
         By default, all labels in `y_true` and `y_pred` are used in sorted order.
 
+        .. versionchanged:: 0.17
+           Parameter `labels` improved for multiclass problem.
+
     pos_label : int, float, bool or str, default=1
         The class to report if `average='binary'` and the data is binary,
         otherwise this parameter is ignored.
         For multiclass or multilabel targets, set `labels=[pos_label]` and
         `average != 'binary'` to report metrics for one label only.
 
-    average : {'binary', 'micro', 'macro', 'samples', 'weighted'}, \
-            default=None
+    average : {'micro', 'macro', 'samples', 'weighted', 'binary'} or None, \
+            default='binary'
+        This parameter is required for multiclass/multilabel targets.
         If ``None``, the metrics for each class are returned. Otherwise, this
         determines the type of averaging performed on the data:
 


Test patch:
diff --git a/sklearn/tests/test_docstring_parameters.py b/sklearn/tests/test_docstring_parameters.py
index 3af463b783bc3..ffa4fe63967f9 100644
--- a/sklearn/tests/test_docstring_parameters.py
+++ b/sklearn/tests/test_docstring_parameters.py
@@ -12,6 +12,7 @@
 import pytest
 
 import sklearn
+from sklearn import metrics
 from sklearn.datasets import make_classification
 
 # make it possible to discover experimental estimators when calling `all_estimators`
@@ -24,6 +25,7 @@
 from sklearn.utils import all_estimators
 from sklearn.utils._testing import (
     _get_func_name,
+    assert_docstring_consistency,
     check_docstring_parameters,
     ignore_warnings,
 )
@@ -320,3 +322,26 @@ def _get_all_fitted_attributes(estimator):
             fit_attr.append(name)
 
     return [k for k in fit_attr if k.endswith("_") and not k.startswith("_")]
+
+
+def test_precision_recall_f_score_docstring_consistency():
+    """Check docstrings parameters of related metrics are consistent."""
+    pytest.importorskip(
+        "numpydoc",
+        reason="numpydoc is required to test the docstrings",
+    )
+    assert_docstring_consistency(
+        [
+            metrics.precision_recall_fscore_support,
+            metrics.f1_score,
+            metrics.fbeta_score,
+            metrics.precision_score,
+            metrics.recall_score,
+        ],
+        include_params=True,
+        # "average" - in `recall_score` we have an additional line: 'Weighted recall
+        # is equal to accuracy.'.
+        # "zero_division" - the reason for zero division differs between f scores,
+        # precison and recall.
+        exclude_params=["average", "zero_division"],
+    )
diff --git a/sklearn/utils/_testing.py b/sklearn/utils/_testing.py
index 1ec38edd20e83..d2a784494e5cd 100644
--- a/sklearn/utils/_testing.py
+++ b/sklearn/utils/_testing.py
@@ -14,12 +14,16 @@
 import shutil
 import sys
 import tempfile
+import textwrap
 import unittest
 import warnings
+from collections import defaultdict, namedtuple
 from collections.abc import Iterable
 from dataclasses import dataclass
+from difflib import context_diff
 from functools import wraps
 from inspect import signature
+from itertools import chain, groupby
 from subprocess import STDOUT, CalledProcessError, TimeoutExpired, check_output
 
 import joblib
@@ -605,6 +609,207 @@ def check_docstring_parameters(func, doc=None, ignore=None):
     return incorrect
 
 
+def _check_item_included(item_name, args):
+    """Helper to check if item should be included in checking."""
+    if args.include is not True and item_name not in args.include:
+        return False
+    if args.exclude is not None and item_name in args.exclude:
+        return False
+    return True
+
+
+def _diff_key(line):
+    """Key for grouping output from `context_diff`."""
+    if line.startswith("  "):
+        return "  "
+    elif line.startswith("- "):
+        return "- "
+    elif line.startswith("+ "):
+        return "+ "
+    elif line.startswith("! "):
+        return "! "
+    return None
+
+
+def _get_diff_msg(docstrings_grouped):
+    """Get message showing the difference between type/desc docstrings of all objects.
+
+    `docstrings_grouped` keys should be the type/desc docstrings and values are a list
+    of objects with that docstring. Objects with the same type/desc docstring are
+    thus grouped together.
+    """
+    msg_diff = ""
+    ref_str = ""
+    ref_group = []
+    for docstring, group in docstrings_grouped.items():
+        if not ref_str and not ref_group:
+            ref_str += docstring
+            ref_group.extend(group)
+        diff = list(
+            context_diff(
+                ref_str.split(),
+                docstring.split(),
+                fromfile=str(ref_group),
+                tofile=str(group),
+                n=8,
+            )
+        )
+        # Add header
+        msg_diff += "".join((diff[:3]))
+        # Group consecutive 'diff' words to shorten error message
+        for start, group in groupby(diff[3:], key=_diff_key):
+            if start is None:
+                msg_diff += "\n" + "\n".join(group)
+            else:
+                msg_diff += "\n" + start + " ".join(word[2:] for word in group)
+        # Add new lines at end of diff, to separate comparisons
+        msg_diff += "\n\n"
+    return msg_diff
+
+
+def _check_consistency_items(items_docs, type_or_desc, section, n_objects):
+    """Helper to check docstring consistency of all `items_docs`.
+
+    If item is not present in all objects, checking is skipped and warning raised.
+    """
+    skipped = []
+    for item_name, docstrings_grouped in items_docs.items():
+        # If item not found in all objects, skip
+        if sum([len(objs) for objs in docstrings_grouped.values()]) < n_objects:
+            skipped.append(item_name)
+        # If more than one key, docstrings not consistent between objects
+        elif len(docstrings_grouped.keys()) > 1:
+            msg_diff = _get_diff_msg(docstrings_grouped)
+            obj_groups = " and ".join(
+                str(group) for group in docstrings_grouped.values()
+            )
+            msg = textwrap.fill(
+                f"The {type_or_desc} of {section[:-1]} '{item_name}' is inconsistent "
+                f"between {obj_groups}:"
+            )
+            msg += msg_diff
+            raise AssertionError(msg)
+    if skipped:
+        warnings.warn(
+            f"Checking was skipped for {section}: {skipped} as they were "
+            "not found in all objects."
+        )
+
+
+def assert_docstring_consistency(
+    objects,
+    include_params=False,
+    exclude_params=None,
+    include_attrs=False,
+    exclude_attrs=None,
+    include_returns=False,
+    exclude_returns=None,
+):
+    """Check consistency between docstring parameters/attributes/returns of objects.
+
+    Checks if parameters/attributes/returns have the same type specification and
+    description (ignoring whitespace) across `objects`. Intended to be used for
+    related classes/functions/data descriptors.
+
+    Entries that do not appear across all `objects` are ignored.
+
+    Parameters
+    ----------
+    objects : list of {classes, functions, data descriptors}
+        Objects to check.
+        Objects may be classes, functions or data descriptors with docstrings that
+        can be parsed by numpydoc.
+
+    include_params : list of str or bool, default=False
+        List of parameters to be included. If True, all parameters are included,
+        if False, checking is skipped for parameters.
+        Can only be set if `exclude_params` is None.
+
+    exclude_params : list of str or None, default=None
+        List of parameters to be excluded. If None, no parameters are excluded.
+        Can only be set if `include_params` is True.
+
+    include_attrs : list of str or bool, default=False
+        List of attributes to be included. If True, all attributes are included,
+        if False, checking is skipped for attributes.
+        Can only be set if `exclude_attrs` is None.
+
+    exclude_attrs : list of str or None, default=None
+        List of attributes to be excluded. If None, no attributes are excluded.
+        Can only be set if `include_attrs` is True.
+
+    include_returns : list of str or bool, default=False
+        List of returns to be included. If True, all returns are included,
+        if False, checking is skipped for returns.
+        Can only be set if `exclude_returns` is None.
+
+    exclude_returns : list of str or None, default=None
+        List of returns to be excluded. If None, no returns are excluded.
+        Can only be set if `include_returns` is True.
+
+    Examples
+    --------
+    >>> from sklearn.metrics import (mean_absolute_error, mean_squared_error,
+    ... median_absolute_error)
+    >>> from sklearn.utils.testing import assert_docstring_consistency
+    ... # doctest: +SKIP
+    >>> assert_docstring_consistency([mean_absolute_error, mean_squared_error],
+    ... include_params=['y_true', 'y_pred', 'sample_weight'])  # doctest: +SKIP
+    >>> assert_docstring_consistency([median_absolute_error, mean_squared_error],
+    ... include_params=True)  # doctest: +SKIP
+    """
+    from numpydoc import docscrape
+
+    Args = namedtuple("args", ["include", "exclude", "arg_name"])
+
+    def _create_args(include, exclude, arg_name, section_name):
+        if exclude and include is not True:
+            raise TypeError(
+                f"The 'exclude_{arg_name}' argument can be set only when the "
+                f"'include_{arg_name}' argument is True."
+            )
+        if include is False:
+            return {}
+        return {section_name: Args(include, exclude, arg_name)}
+
+    section_args = {
+        **_create_args(include_params, exclude_params, "params", "Parameters"),
+        **_create_args(include_attrs, exclude_attrs, "attrs", "Attributes"),
+        **_create_args(include_returns, exclude_returns, "returns", "Returns"),
+    }
+
+    objects_doc = dict()
+    for obj in objects:
+        if (
+            inspect.isdatadescriptor(obj)
+            or inspect.isfunction(obj)
+            or inspect.isclass(obj)
+        ):
+            objects_doc[obj.__name__] = docscrape.NumpyDocString(inspect.getdoc(obj))
+        else:
+            raise TypeError(
+                "All 'objects' must be one of: function, class or descriptor, "
+                f"got a: {type(obj)}."
+            )
+
+    n_objects = len(objects)
+    for section, args in section_args.items():
+        type_items = defaultdict(lambda: defaultdict(list))
+        desc_items = defaultdict(lambda: defaultdict(list))
+        for obj_name, obj_doc in objects_doc.items():
+            for item_name, type_def, desc in obj_doc[section]:
+                if _check_item_included(item_name, args):
+                    # Normalize white space
+                    type_def = " ".join(type_def.strip().split())
+                    desc = " ".join(chain.from_iterable(line.split() for line in desc))
+                    # Use string type/desc as key, to group consistent objs together
+                    type_items[item_name][type_def].append(obj_name)
+                    desc_items[item_name][desc].append(obj_name)
+
+        _check_consistency_items(type_items, "type specification", section, n_objects)
+        _check_consistency_items(desc_items, "description", section, n_objects)
+
+
 def assert_run_python_script_without_output(source_code, pattern=".+", timeout=60):
     """Utility to check assertions in an independent Python subprocess.
 
diff --git a/sklearn/utils/tests/test_testing.py b/sklearn/utils/tests/test_testing.py
index 4e7a40dae1222..08ded6e404924 100644
--- a/sklearn/utils/tests/test_testing.py
+++ b/sklearn/utils/tests/test_testing.py
@@ -15,6 +15,7 @@
     _get_warnings_filters_info_list,
     assert_allclose,
     assert_allclose_dense_sparse,
+    assert_docstring_consistency,
     assert_run_python_script_without_output,
     check_docstring_parameters,
     create_memmap_backed_data,
@@ -404,7 +405,6 @@ def test_check_docstring_parameters():
         reason="numpydoc is required to test the docstrings",
         minversion="1.2.0",
     )
-
     incorrect = check_docstring_parameters(f_ok)
     assert incorrect == []
     incorrect = check_docstring_parameters(f_ok, ignore=["b"])
@@ -534,6 +534,269 @@ def test_check_docstring_parameters():
         assert msg == incorrect, '\n"%s"\n not in \n"%s"' % (msg, incorrect)
 
 
+def f_one(a, b):  # pragma: no cover
+    """Function one.
+
+    Parameters
+    ----------
+    a : int,   float
+        Parameter a.
+        Second    line.
+
+    b : str
+        Parameter b.
+
+    Returns
+    -------
+    c : int
+       Returning
+
+    d : int
+       Returning
+    """
+    pass
+
+
+def f_two(a, b):  # pragma: no cover
+    """Function two.
+
+    Parameters
+    ----------
+    a :   int, float
+        Parameter a.
+          Second line.
+
+    b : str
+        Parameter bb.
+
+    e : int
+        Extra parameter.
+
+    Returns
+    -------
+    c : int
+       Returning
+
+    d : int
+       Returning
+    """
+    pass
+
+
+def f_three(a, b):  # pragma: no cover
+    """Function two.
+
+    Parameters
+    ----------
+    a :   int, float
+        Parameter a.
+
+    b : str
+        Parameter B!
+
+    e :
+        Extra parameter.
+
+    Returns
+    -------
+    c : int
+       Returning.
+
+    d : int
+       Returning
+    """
+    pass
+
+
+def test_assert_docstring_consistency_object_type():
+    """Check error raised when `objects` incorrect type."""
+    pytest.importorskip(
+        "numpydoc",
+        reason="numpydoc is required to test the docstrings",
+    )
+    with pytest.raises(TypeError, match="All 'objects' must be one of"):
+        assert_docstring_consistency(["string", f_one])
+
+
+@pytest.mark.parametrize(
+    "objects, kwargs, error",
+    [
+        (
+            [f_one, f_two],
+            {"include_params": ["a"], "exclude_params": ["b"]},
+            "The 'exclude_params' argument",
+        ),
+        (
+            [f_one, f_two],
+            {"include_returns": False, "exclude_returns": ["c"]},
+            "The 'exclude_returns' argument",
+        ),
+    ],
+)
+def test_assert_docstring_consistency_arg_checks(objects, kwargs, error):
+    """Check `assert_docstring_consistency` argument checking correct."""
+    pytest.importorskip(
+        "numpydoc",
+        reason="numpydoc is required to test the docstrings",
+    )
+    with pytest.raises(TypeError, match=error):
+        assert_docstring_consistency(objects, **kwargs)
+
+
+@pytest.mark.parametrize(
+    "objects, kwargs, error, warn",
+    [
+        pytest.param(
+            [f_one, f_two], {"include_params": ["a"]}, "", "", id="whitespace"
+        ),
+        pytest.param([f_one, f_two], {"include_returns": True}, "", "", id="incl_all"),
+        pytest.param(
+            [f_one, f_two, f_three],
+            {"include_params": ["a"]},
+            (
+                r"The description of Parameter 'a' is inconsistent between "
+                r"\['f_one',\n'f_two'\]"
+            ),
+            "",
+            id="2-1 group",
+        ),
+        pytest.param(
+            [f_one, f_two, f_three],
+            {"include_params": ["b"]},
+            (
+                r"The description of Parameter 'b' is inconsistent between "
+                r"\['f_one'\] and\n\['f_two'\] and"
+            ),
+            "",
+            id="1-1-1 group",
+        ),
+        pytest.param(
+            [f_two, f_three],
+            {"include_params": ["e"]},
+            (
+                r"The type specification of Parameter 'e' is inconsistent between\n"
+                r"\['f_two'\] and"
+            ),
+            "",
+            id="empty type",
+        ),
+        pytest.param(
+            [f_one, f_two],
+            {"include_params": True, "exclude_params": ["b"]},
+            "",
+            r"Checking was skipped for Parameters: \['e'\]",
+            id="skip warn",
+        ),
+    ],
+)
+def test_assert_docstring_consistency(objects, kwargs, error, warn):
+    """Check `assert_docstring_consistency` gives correct results."""
+    pytest.importorskip(
+        "numpydoc",
+        reason="numpydoc is required to test the docstrings",
+    )
+    if error:
+        with pytest.raises(AssertionError, match=error):
+            assert_docstring_consistency(objects, **kwargs)
+    elif warn:
+        with pytest.warns(UserWarning, match=warn):
+            assert_docstring_consistency(objects, **kwargs)
+    else:
+        assert_docstring_consistency(objects, **kwargs)
+
+
+def f_four(labels):  # pragma: no cover
+    """Function four.
+
+    Parameters
+    ----------
+
+    labels : array-like, default=None
+        The set of labels to include when `average != 'binary'`, and their
+        order if `average is None`. Labels present in the data can be excluded.
+    """
+    pass
+
+
+def f_five(labels):  # pragma: no cover
+    """Function five.
+
+    Parameters
+    ----------
+
+    labels : array-like, default=None
+        The set of labels to include when `average != 'binary'`, and their
+        order if `average is None`. This is an extra line. Labels present in the
+        data can be excluded.
+    """
+    pass
+
+
+def f_six(labels):  # pragma: no cover
+    """Function six.
+
+    Parameters
+    ----------
+
+    labels : array-like, default=None
+        The group of labels to add when `average != 'binary'`, and the
+        order if `average is None`. Labels present on them datas can be excluded.
+    """
+    pass
+
+
+def test_assert_docstring_consistency_error_msg():
+    """Check `assert_docstring_consistency` difference message."""
+    pytest.importorskip(
+        "numpydoc.docscrape",
+        reason="numpydoc is required to test the docstrings",
+    )
+    msg = r"""The description of Parameter 'labels' is inconsistent between
+\['f_four'\] and \['f_five'\] and \['f_six'\]:
+
+\*\*\* \['f_four'\]
+--- \['f_five'\]
+\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*
+
+\*\*\* 10,25 \*\*\*\*
+
+--- 10,30 ----
+
+  'binary'`, and their order if `average is None`.
+\+ This is an extra line.
+  Labels present in the data can be excluded.
+
+\*\*\* \['f_four'\]
+--- \['f_six'\]
+\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*
+
+\*\*\* 1,25 \*\*\*\*
+
+  The
+! set
+  of labels to
+! include
+  when `average != 'binary'`, and
+! their
+  order if `average is None`. Labels present
+! in the data
+  can be excluded.
+--- 1,25 ----
+
+  The
+! group
+  of labels to
+! add
+  when `average != 'binary'`, and
+! the
+  order if `average is None`. Labels present
+! on them datas
+  can be excluded."""
+
+    with pytest.raises(AssertionError, match=msg):
+        assert_docstring_consistency([f_four, f_five, f_six], include_params=True)
+
+
 class RegistrationCounter:
     def __init__(self):
         self.nb_calls = 0