Docs: Show typing info and parse INPUT/OUTPUT sections #40634
Conversation
This reverts commit 5ae3584.
… cannot be parsed (for now)
|
Documentation preview for this PR (built with commit bd9b771; changes) is ready! 🎉 |
|
There is something odd with the rendering: some of the items below For example, in https://doc-pr-40634--sagemath.netlify.app/html/en/reference/combinat/sage/combinat/bijectionist |
Some formatting inconsistencies here. The default value of Can you apply whatever formatting you applied to |
This might be because what the docstring gives as the default value of |
|
The parsing for https://doc-pr-40634--sagemath.netlify.app/html/en/reference/combinat/sage/combinat/bijectionist#sage-combinat-bijectionist looks mostly correct now, except that, apparently, is turned into which is confusing. |
That comes from the fact that I should also note that the typing extension has the ability to automatically add "default: xyz" to the parameter description in the docs based on the function signature. I've disabled this for now, since otherwise one gets a duplication when the existing docs already point out the default value. Any suggestions on how to proceed here? Perhaps, it's okay to accept that duplication until someone finds the time to cleanup the docstring?
Right, thanks. Gosh, it only took me 5 years to finally work on this ;-)
I agree this is a bit confusing and suboptimal. I oriented myself here at the numpy doc style (https://numpydoc.readthedocs.io/en/latest/format.html#parameters), which allow to specify the description in a similar way: And according to https://github.com/sphinx-doc/sphinx/blob/e8ab5cf1c57d106f57cb6c77b60dc4a5ae2c9a37/tests/test_extensions/test_ext_napoleon_docstring.py#L1696-L1699 so a complete duplication of the description. I think the reason is that the type could be different, e.g. def print(a: int, b: float):
"""
INPUT:
a,b -- value to be printed
"""would need separate bullet points for I clicked around the docs and I'm quite happy how they look now. Could very well be that certain docstrings are not parsed completely correctly - if you see any, let me know and I'll fix them still here. Otherwise we can always improve things later. |
Is is possible to add the default value from the function signature only if a default value is not specified? And then update the relevant part of the developer guide to say that docstrings should not include the default value unless they need to override what the function signature says. Ideally we should never need to override the default value in the function signature in the docstring, but I'm sure there's some weird edge case I'm not thinking of. When updating the developer guide I'd also mention that we previouslyincluded default values in docstrings but it is no longer needed and they can be removed as part of refactoring/formatting now.
These two look weird, the parameter is described by a bulleted list which I think is causing formatting issues. There are probably other docstrings with this issue as well:
|
So, how should this be fixed (in this particular instance)? I wouldn't be surprised if several parameters that share descriptions (possibly depending on each other) occur frequently. It would be good to have a standard pattern for this situation. Note that, in the case above, it will be hard for the reader to figure out what is meant. |
Maybe, but this looks somewhat complicated. One could patch
This is sadly a sphinx bug, see sphinx-doc/sphinx#4220, sphinx-doc/sphinx#2768. (Side note: at least the rational_points description looks better than with the current develop branch: https://doc-develop--sagemath.netlify.app/html/en/reference/curves/sage/schemes/curves/affine_curve.html#sage.schemes.curves.affine_curve.AffinePlaneCurve_finite_field.rational_points)
If it's really the same description, you can use a comma between the paramaters like in If the description is actually not the same, one needs to use the more verbose way: In general, there is a slight change of perspective. Sage's INPUT is more used to generally describe the input of the function as a whole, while Sphinx's Parameters section is a list of parameters, that then each are described in detail. This is not possible with Sphinx parameter list and needs to be reworked to display nicely. |
The That said, if it's too complicated we can leave that for future work. The default value shows up in the function signature in the docs anyway. This next question is somewhat open-ended, not sure if there's a right answer. The style guide says this about types in docstrings:
This is because sometimes the typing information is more technical than users might need, or the actual valid inputs are more restrictive than the type (if the function accepts a prime integer then the type is still just Consider the parameter Either way, I think with this PR it is appropriate to update the General Conventions in our Developer Guide to say that the type information can be omitted from the docstring if it is included in a type annotation, at least for basic Python types like |
I think this would be a nice addition, but would leave it for a follow-up PR. The issue you raise about the various forms of integers is a good one, and I don't really have a solution for now. My intuition would be to create a type alias (say IntegerLike = int | Integer | some other ones?), document and use that alias then in other functions. But as your example showed, there might be need for further specialized "integer" type alias, or perhaps a completely different solution. Maybe the problem and solution becomes crystalizes itself once we gain more experience with the typing system.
Good point, done now. |
That description looks good. I would like to see something added about the
Sometimes we actually want only one of This is also complicated by differences between Sage the programming language and Sage the Python library. When used as a Python library I think we should use generally use If there is some function where we expect both will be used by Sage itself, or where we want to explicitly advertise |
|
I generally agree, but would say that Personally, the main advantage of the typing info is for the convenience while developing for sage. Thus, I would say the typing info should cater for the use case of "Sage the Python library". (sage the language also doesn't support type checking, right?) Do you have a suggestion where one should add the type alias for |
While developing for Sage (i.e. what we do in this GitHub repo), I think we should usually be using
I assume you only mean for parameters, not returns types, since the return type should usually be one of the two (unless perhaps if it returns the same type that was input, but I'd prefer if that was rare). I'm still not entirely convinced the default for parameters should be Wouldn't this make compatibility with
I think I would make a new file for all type aliases, maybe Something like this:
"""This module defines and documents common type aliases and unions used in Sage."""
from typing import TYPE_CHECKING
if TYPE_CHECKING:
# Import modules, define type unions, aliases, etc.
# All those imports might be expensive at runtime, hence why this is wrapped in `if TYPE_CHECKING`
type Int = Integer | int
"""Type alias for Sage integers or Python integers, used to annotate parameters of functions that accept both."""
type ModInt = Int | IntegerMod_abstract
"""Type alias for types that can be used in Sage's modular arithmetic."""
# etc. |
|
If we want to force people to also wrap the call to else:
assert False, 'typing.py can only be imported in an `if TYPE_CHECKING:` block, not at runtime'The overhead of importing |
There was a problem hiding this comment.
Since there is pending work (also testing the labels for https://groups.google.com/g/sage-devel/c/jTpHiM7YmTM)
Add the typing information that is present in the method signature automatically to the docs.
So for
def format_unit(value: float, unit: str) -> str:both the type of the parameters, as well as the return type are extracted and shown in the docs. This is done using the https://github.com/tox-dev/sphinx-autodoc-typehints package. Fixes #30894.For this to work, one needs to parse Sage's custom docstring syntax and convert them to standard sphinx commands like
:params. This is accomplished in the newly added sage transform extension. As a nice byproduct the headers of these sections are now shown as intended by the furo style (ie as headings), and one additionally gets stronger uniformity as the docstrings are now syntax-checked (at least to some degree).The parsing also works for other sections (like authors and references). Similar work in this direction had be done in #37614).
A good example that already contains quite a few typing info is at https://doc-pr-40634--sagemath.netlify.app/html/en/reference/manifolds/sage/manifolds/differentiable/symplectic_form.html#sage.manifolds.differentiable.symplectic_form.SymplecticForm.
📝 Checklist
⌛ Dependencies