Skip to content

Documentation shows incorrect default values for keyword-only arguments #39627

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
2 tasks done
DaveWitteMorris opened this issue Mar 4, 2025 · 3 comments · May be fixed by #39774
Open
2 tasks done

Documentation shows incorrect default values for keyword-only arguments #39627

DaveWitteMorris opened this issue Mar 4, 2025 · 3 comments · May be fixed by #39774

Comments

@DaveWitteMorris
Copy link
Member

DaveWitteMorris commented Mar 4, 2025

Steps To Reproduce

The signature of the method sage.categories.enumerated_sets.EnumeratedSets.ParentMethods.map is map(self, f, name=None, *, is_injective=True), but the documentation shows map(f, name, is_injective=None). (See this doc page.)

Expected Behavior

The documentation should show the correct signature of the method.

Actual Behavior

There are two problems with the signature that is presented in the documentation:

  1. The default value for the name argument is erroneously given to is_injective instead.
  2. There is no indication that is_injective is a keyword-only argument.

Additional Information

The problems are not unique to this method -- they also show up in the documentation of other methods that have a keyword-only argument, such as sage.combinat.designs.bibd.BalancedIncompleteBlockDesign.arc. It appears to me that sphinx/autodoc is shifting the default values one step to the right.

Environment

  • OS: any(?)
  • Sage Version: 10.5 (but probably not important)

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@DaveWitteMorris
Copy link
Member Author

Partial diagnosis. The problem seems to be in sage.misc.sageinspect.sage_getargspec. It does not seem to realize that arguments can be keyword-only (and does not pick up their defaults). For example:

sage: def afunc(a, b=1, *, x=2, y, z=3):
....:     return
....: 
sage: from sage.misc.sageinspect import sage_getargspec
sage: sage_getargspec(afunc)
FullArgSpec(args=['a', 'b', 'x', 'y', 'z'], varargs=None, varkw=None, defaults=(1,), 
kwonlyargs=[], kwonlydefaults=None, annotations={})
sage: from sage.misc.sageinspect import sage_formatargspec
sage: sage_formatargspec(*sage_getargspec(afunc))
'(a, b, x, y, z=1)'

The sage_getargspec method needs to be fixed (and needs doctests!).

By the way, I don't think the positional-only separator "/" is being handled, either, but it doesn't seem to arise in the sagemath source code yet.

@Biffo89 Biffo89 linked a pull request Mar 23, 2025 that will close this issue
5 tasks
@Biffo89
Copy link
Contributor

Biffo89 commented Mar 23, 2025

Hi, I'm new to contributing to sage but have had a look at this. The functions inspect.getargs and inspect.GetFullArgSpec are deprecated and I have replaced them in one block with inspect.getfullargspec, which returns correct signatures. I've tested this on the two pages mentioned and also on the example afunc given. There are some further uses of deprecated functions elsewhere in this function, but I am unsure of what would trigger these branches so have left them as is.

I have a PR #39774 ready for review

@DaveWitteMorris
Copy link
Member Author

I don't think this is a good first issue, because I think the fix is probably not straightforward, and will need to be done with care. Did you read these paragraphs in the docstring of sage_formatargspec?

    This is taken from Python 3.7's inspect.py, where it is
    deprecated. The only change, aside from documentation (this
    paragraph and the next, plus doctests), is to remove the
    deprecation warning.

    Sage uses this function to format arguments, as obtained by
    :func:`sage_getargspec`. Since :func:`sage_getargspec` works for
    Cython functions while Python's inspect module does not, it makes
    sense to keep this function for formatting instances of
    ``inspect.FullArgSpec``.

The methods need to work for cython functions, not just python functions.

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

Successfully merging a pull request may close this issue.

2 participants