Skip to content

Allow multiple Warning(s) sections #594

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

Open
lucascolley opened this issue Dec 15, 2024 · 11 comments
Open

Allow multiple Warning(s) sections #594

lucascolley opened this issue Dec 15, 2024 · 11 comments

Comments

@lucascolley
Copy link
Contributor

Using the specified Warnings section I can get something that looks like this:

image

It would be nice to be able to split this into two separate admonitions. It feels natural to me to have two separate headers, both titled Warning.

@lucascolley
Copy link
Contributor Author

It seems like sphinx.ext.napoleon supports this: https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#docstring-sections

@lucascolley
Copy link
Contributor Author

napoleon parses warning blocks as admonitions (at least for GoogleDocstring): https://github.com/sphinx-doc/sphinx/blob/c4929d026c8d22ba229b39cfc2250a9eb1476282/sphinx/ext/napoleon/docstring.py#L400

@rossbar
Copy link
Contributor

rossbar commented May 7, 2025

IMO the Warnings section shouldn't exist... the sphinx built-in .. warning:: directive is preferable. Since it does exist it may be straightforward to just parse the Warnings heading(s) and replace it with .. warning::.

@stefanv
Copy link
Contributor

stefanv commented May 7, 2025

It does feel like Warnings should have been the spelling for Warns. But, given that we didn't do that, I agree with @rossbar's assessment.

@larsoner
Copy link
Collaborator

larsoner commented May 7, 2025

To me this seems a bit off from the stated purpose of the section, which to me is to enumerate and describe the Warning subclasses that can be warnings.warn'ed by the called function. From the doc:

An optional section detailing which warnings get raised and under what conditions, formatted similarly to Raises.

So the idea is that the section should be similar to the Raises one, e.g., something like:

Warnings
--------
RuntimeWarning
    Emitted if the denominator is zero. In this case, NaN will be returned.
UserWarning
    Emitted when blah blah blah.

With this in mind, there is some overlap potentially with when people would want to use the .. warning:: admonition sometimes, but to me the goals are not sufficiently distinct that I think we should keep the Warnings section.

More specific to the OP's use case, I think they should use .. warning:: for that whole box somewhere in the Notes subsection, since it has to do with a potential usage "gotcha" rather than with what warnings can be emitted by calling the function. So for that use case, there would no Warnings subheading use at all.

If this makes sense, we should probably add some text to clarify the intended use of the Warnings section.

@larsoner
Copy link
Collaborator

larsoner commented May 7, 2025

Ooof wait I was reading the description of the Warns section! Agreed that Warnings probably does not make sense 🤦

@lucascolley
Copy link
Contributor Author

lucascolley commented May 7, 2025

Yes, this is about Warnings, not Warns 👍

IMO the Warnings section shouldn't exist... the sphinx built-in .. warning:: directive is preferable.

they should use .. warning:: for that whole box somewhere in the Notes subsection, since it has to do with a potential usage "gotcha" rather than with what warnings can be emitted by calling the function. So for that use case, there would no Warnings subheading use at all.

I think I tried this initially, and IIRC it worked under the Notes header, but not under other headers. The problem with that is that one might want the warnings to show higher up the page than Notes, e.g. right below Parameters (or at least, above Notes and See Also, as per the current spec).

I might be misremembering, though—feel free to play around with the source of https://data-apis.org/array-api-extra/generated/array_api_extra.at.html#array_api_extra.at and pixi run docs if there is an easier solution 👍

@rossbar
Copy link
Contributor

rossbar commented May 7, 2025

I think I tried this initially, and IIRC it worked under the Notes header, but not under other headers. The problem with that is that one might want the warnings to show higher up the page than Notes, e.g. right below Parameters (or at least, above Notes and See Also, as per the current spec).

This is another reason to prefer the .. warning:: directive. I haven't tested, but in principle this should show up at the exact location in the docstring where you place it. In other words if you have something like:

def foo(x, y):
    """foo's basic description.

    Foo's extended description

    .. warning:: ``foo`` is way too common a name.

    Parameters
    ----------
    x : scalar
        A value
        .. warning:: ``x`` is also too generic
    y : scalar
    
    Notes
    -----
    This function is an example to show multiple warnings in
    docstrings.
    
    .. warning:: here's another

Then you should get three separate warning admonitions rendered in correct locations (the extended summary, the parameter description, and the notes section, respectively)

If you instead rely on a Warnings section, numpydoc will grab that and move it to the "Warnings" location in the docstring (i.e. after Warns but before See Also).

Without having looked at it too closely, I suspect it would be possible to break this pattern to allow multiple Warnings sections and preserve their location in the docstring, but I'd much prefer to "deprecate by documentation" instead and just recommend folks use the .. warning:: directive instead of the Warnings headaing.

@lucascolley
Copy link
Contributor Author

I tried:

    Returns
    -------
    Updated input array.

    .. warning::

        When you omit the ``copy`` parameter, you should never reuse the parameter
        array later on; ideally, you should reassign it immediately::

            >>> import array_api_extra as xpx
            >>> x = xpx.at(x, 0).set(2)

        The above best practice pattern ensures that the behaviour won't change depending
        on whether ``x`` is writeable or not, as the original ``x`` object is dereferenced
        as soon as ``xpx.at`` returns; this way there is no risk to accidentally update it
        twice.

But that doesn't work:

Image

@lucascolley
Copy link
Contributor Author

lucascolley commented May 7, 2025

it may be important to mention that I am using "sphinx.ext.napoleon" rather than numpydoc for the docstring processing. Full list of extensions:

extensions = [
    "myst_parser",
    "sphinx.ext.autodoc",
    "sphinx.ext.autosummary",
    "sphinx.ext.intersphinx",
    "sphinx.ext.mathjax",
    "sphinx.ext.napoleon",
    "sphinx_autodoc_typehints",
    "sphinx_copybutton",
]

@rossbar
Copy link
Contributor

rossbar commented May 7, 2025

The .. warnings:: directive is a sphinx built-in, and should "just work" wherever it's placed. I've opened a dummy PR for illustration purposes where I've added .. warning:: 's at various places (diff). AFICT they render as expected.

I'm not sure what's happening in your case - my guesses would be a) a subtle indentation bug (though I don't see any problems) followed by b) different behavior in the napoleon parser, whether intentional or not.

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

No branches or pull requests

4 participants