Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression in 3.1.0: docstring of ...:1:duplicate object description of ..., other instance in ..., use :noindex: for one of them #7817

Open
StrikerRUS opened this issue Jun 10, 2020 · 13 comments

Comments

@StrikerRUS
Copy link

Describe the bug
Incompatibility with numpy docstring convention.

Attributes that are properties and have their own docstrings can be simply listed by name:
https://numpydoc.readthedocs.io/en/latest/format.html#class-docstring

To Reproduce
Toy example:

conf.py:

import os
import sys
import sphinx


CURR_PATH = os.path.abspath(os.path.dirname(__file__))
LIB_PATH = os.path.join(CURR_PATH, os.path.pardir, 'python-package')
sys.path.insert(0, LIB_PATH)


# -- General configuration ------------------------------------------------

# The master toctree document.
master_doc = 'index'

# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
# This patterns also effect to html_static_path and html_extra_path
exclude_patterns = ['_build', 'Thumbs.db', '.DS_Store']

# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions = [
    'sphinx.ext.autodoc',
    'sphinx.ext.autosummary',
    'sphinx.ext.todo',
    'sphinx.ext.viewcode',
    'sphinx.ext.napoleon',
]

autodoc_default_flags = ['members', 'inherited-members', 'show-inheritance']
autodoc_default_options = {
    "members": True,
    "inherited-members": True,
    "show-inheritance": True,
}

# Generate autosummary pages. Output should be set with: `:toctree: pythonapi/`
autosummary_generate = ['Python-API.rst']

# Only the class' docstring is inserted.
autoclass_content = 'class'

# If true, `todo` and `todoList` produce output, else they produce nothing.
todo_include_todos = False

index.rst:

.. toctree::
   :caption: Contents:

   Python API <Python-API>

Python-API.rst:

Python API
==========

.. currentmodule:: lightgbm


Scikit-learn API
----------------

.. autosummary::
    :toctree: pythonapi/

    A

..\python-package\lightgbm\sklearn.py:

class A(object):
    """Class A."""

    def __init__(self, x=42):
        """Init description.

        Parameters
        ----------
        x : int, optional (default=42)
            X description.

        Attributes
        ----------
        n_features_
        classes_ : array of shape = [n_classes]
            The class label array (only for classification problem).
        """
        self.x = x
        self._n_features = None
        self._n_classes = None


    @property
    def n_features_(self):
        """N features description."""
        return self._n_features

    @property
    def classes_(self):
        return 42

Expected behavior
No warnings.

Your project
https://github.com/microsoft/LightGBM/blob/master/docs/conf.py

Environment info

@StrikerRUS StrikerRUS changed the title regression in 3.1.0: docstring of lightgbm.LGBMClassifier.best_iteration_:1:duplicate object description of lightgbm.LGBMClassifier.best_iteration_, other instance in pythonapi/lightgbm.LGBMClassifier, use :noindex: for one of them regression in 3.1.0: docstring of ...:1:duplicate object description of ..., other instance in ..., use :noindex: for one of them Jun 10, 2020
@tk0miya
Copy link
Member

tk0miya commented Jun 11, 2020

It seems A. n_features_ is documented twice. The first time is in the docstring of A.__init__(). And the second one is the docstring of A.n_features_ property itself. I don't know why past versions do not warn this case. But it seems duplicated. So the warning is correct to me.
How about removing either one?

@tk0miya tk0miya added this to the 3.1.1 milestone Jun 11, 2020
@StrikerRUS
Copy link
Author

Hi @tk0miya !

Thanks a lot for your reply!

It seems A. n_features_ is documented twice.

Yes, indeed. But it is the correct way to document properties in numpy convention. Please refer to

Attributes that are properties and have their own docstrings can be simply listed by name:

Attributes
----------
real
imag
x : float
    The X coordinate
y : float
    The Y coordinate

https://numpydoc.readthedocs.io/en/latest/format.html#class-docstring

@tk0miya
Copy link
Member

tk0miya commented Jun 12, 2020

You should not add a docstring to A.n_features_ property if you'd like to document n_features_ in the docstring of A.__init__(). They're conflicted. Is there some reason to keep it as is? If you'd not like to change it, please append :meta private: to the docstring of A.n_features_. Then the property will be suppressed on output.

@StrikerRUS
Copy link
Author

@tk0miya Many thanks for the proposed workarounds! Can you please clarify what's the problem to follow numpy guide and why previous Sphinx versions had no problems with it?

@tk0miya
Copy link
Member

tk0miya commented Jun 12, 2020

There are no problem to follow the numpy guide. That is a misunderstanding. Only what I said is you have written a document for A.n_features_ via two ways; the docstring of A.__init__() and the docstring of A.n_features_ itself. I still don't know why old Sphinx does not raise a warning for this duplication. But I believe current behavior is correct. In other word, old sphinx has a bug about not warning for such duplication.

@StrikerRUS
Copy link
Author

But I believe current behavior is correct.

I don't think so, because numpy guide allows a such type of duplication. However, I'm OK to remove documentation from __init__. So feel free to close the issue if you think that Sphinx shouldn't respect that paragraph from numpy docs I quoted early, because for me personally this won't be an issue anymore.

@tk0miya
Copy link
Member

tk0miya commented Jun 13, 2020

The numpy guide only says the docstring of __init__() method can describe attributes (including properties) of the object. It does not mention about conflicts with the docstring of property itself. So it is better to remove the docstring of the A.n_features_ if you'd like to follow the numpy guide. Of course, it's okay to remove it from __init__().

@StrikerRUS
Copy link
Author

So it is better to remove the docstring of the A.n_features_

Unfortunately, it is not an option due to D102 | Missing docstring in public method error of pydocstyle analysis tool for PEP 257.

It does not mention about conflicts with the docstring of property itself.

It should be no conflicts in case __init__ has only a name of a property and all other information is handled in property's docstring.

@tk0miya
Copy link
Member

tk0miya commented Jun 14, 2020

Hmm... Okay, I'll reconsider this again (for 3.2). So I keep this open for a while.

@tk0miya
Copy link
Member

tk0miya commented Aug 14, 2020

I reconsider this. If you'd like to write attributes to the docstring of class (or __init__() method) following spec of numpydoc, you should not create a document for attributes because of duplication. At first, I just thought not to write docstrings to each attributes (and properties) as commented above. But, I found another approach. It is updating the template of autosummary. The default class template of autosummary shows all of attributes of the class like this:

{{ fullname | escape | underline}}

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}

   {% block methods %}
   .. automethod:: __init__

   {% if methods %}
   .. rubric:: {{ _('Methods') }}

   .. autosummary::
   {% for item in methods %}
      ~{{ name }}.{{ item }}
   {%- endfor %}
   {% endif %}
   {% endblock %}

Then the attributes are only described in the docstring of the class (or __init__() method).

Note: This approach is better for this case. But it does not work properly at this moment. Because autodoc considers properties are kind of methods now. We have a plan to change the behavior in (nearly) future. After that, it will be resolved via customized autosummary template, I believe.

I'll check my idea will really work well after the update. This is a rough implementation of my idea:

FROM python:3.8-slim

RUN apt update; apt install -y build-essential curl git make unzip vim
RUN git clone https://github.com/microsoft/LightGBM
WORKDIR /LightGBM/docs
RUN git checkout fa2de89bb6a8d9baae164e50a25ad82ff2d340a4~1
RUN pip install -r requirements_base.txt
RUN pip install Sphinx==3.1.2
ENV C_API=NO
RUN mkdir -p _templates/autosummary
RUN { \
        echo "{{ fullname | escape | underline}}"; \
        echo ""; \
        echo ".. currentmodule:: {{ module }}"; \
        echo ""; \
        echo ".. autoclass:: {{ objname }}"; \
        echo ""; \
        echo "   {% block methods %}"; \
        echo "   .. automethod:: __init__"; \
        echo ""; \
        echo "   {% if methods %}"; \
        echo "   .. rubric:: {{ _('Methods') }}"; \
        echo ""; \
        echo "   .. autosummary::"; \
        echo "   {% for item in methods %}"; \
        echo "      ~{{ name }}.{{ item }}"; \
        echo "   {%- endfor %}"; \
        echo "   {% endif %}"; \
        echo "   {% endblock %}"; \
    } > _templates/autosummary/class.rst
RUN echo "templates_path = ['_templates']" >> conf.py
RUN echo "exclude_patterns.append('_templates/**/*.rst')" >> conf.py
RUN make html

@tk0miya tk0miya modified the milestones: 3.5.0, 4.0.0 Feb 4, 2021
@tk0miya tk0miya modified the milestones: 4.0.0, 4.1.0 Apr 19, 2021
@kcharlie2
Copy link

kcharlie2 commented Jun 22, 2021

I have experienced this bug as well, but would like some clarification. Is this for attributes documented in __init__ only, or also the class definition?

In the numpy example https://numpydoc.readthedocs.io/en/latest/format.html#class-docstring, they show documenting attributes in the class definition, not __init__.

I am getting the same duplicate object description warning listing attribute names in the class's Attributes section, and documenting them in the @property, which is exactly what the example shows.

@tk0miya tk0miya modified the milestones: 4.1.0, 4.2.0 Jul 10, 2021
@tk0miya tk0miya modified the milestones: 4.2.0, 4.3.0 Sep 12, 2021
@tk0miya tk0miya modified the milestones: 4.3.0, 4.4.0 Nov 8, 2021
@tk0miya tk0miya modified the milestones: 4.4.0, 5.0.0 Jan 12, 2022
@Fl4m3Ph03n1x
Copy link

Fl4m3Ph03n1x commented Apr 28, 2022

I am currently facing the same issue:

@dataclass(frozen=True)
class Feature:
    """
    Structured payload of a Feature...

    Attributes
    ---------
    name : str
        The name of this ``Feature``.
    enabled : bool
        Whether or not this ``Feature`` is enabled....
    """
    name: str
    enabled: bool

Generates the same duplication warning.
I am also trying to follow the same numpydoc convention as @kcharlie2 .

If I change to Attributes to Parameters, the warning goes away, but I still get annoying duplication in my generated HTML for no good reason:

doc

Is there now a way to fix this and still follow the numpydoc convention?

@tk0miya tk0miya modified the milestones: 5.0.0, 5.x May 2, 2022
@AA-Turner AA-Turner modified the milestones: 5.x, 6.x Oct 4, 2022
ymd-h added a commit to ymd-h/vulkpy that referenced this issue Mar 11, 2023
Since sphinx cannot handle `Attributes` section of `dataclass` correctly,
we decided not to use `dataclass`.

Ref: sphinx-doc/sphinx#7817 (comment)
@AA-Turner AA-Turner modified the milestones: 6.x, 7.x Apr 29, 2023
@Conchylicultor
Copy link

+1, @dataclass documentation raise: WARNING: duplicate object description of MyDataclass.my_field, ..., use :noindex: for one of them, as pointed out by @Fl4m3Ph03n1x.

Is there a workaround ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants