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

ENH: Restore numpy.lib.add_newdoc_ufunc #26233

Open
lpsinger opened this issue Apr 8, 2024 · 11 comments
Open

ENH: Restore numpy.lib.add_newdoc_ufunc #26233

lpsinger opened this issue Apr 8, 2024 · 11 comments
Milestone

Comments

@lpsinger
Copy link
Contributor

lpsinger commented Apr 8, 2024

Proposed new feature or change:

I maintain several Python packages with C extensions that define ufuncs and generalized ufuncs. I have been relying upon the add_newdoc_ufunc function to add docstrings to those functions because it is such a pain to write legible multiline strings in C. According to the Numpy 2.0.0 migration guide, add_newdoc_ufunc is "an internal function and doesn’t have a replacement." Is it possible to add it back to the public namespace as numpy.lib.add_newdoc_ufunc?

@charris charris added this to the 2.0.0 release milestone Apr 8, 2024
@mattip
Copy link
Member

mattip commented Apr 9, 2024

This function messes with the PyTypeObject's tp_doc attribute after PyType_Ready is called, which is problematic. See #10167. To create multiline docstrings, couldn't you add a \n to the end of the C string?

@seberg
Copy link
Member

seberg commented Apr 9, 2024

Not sure how much you need it, I am tempted to say: just use the private version. We will keep pretending that we can just delete it at some point (i.e. doing this break in some future), but I don't expect that future to happen very soon.

@lpsinger
Copy link
Contributor Author

lpsinger commented Apr 9, 2024

What about a patch to add a setter for the doc property?

@seberg
Copy link
Member

seberg commented Apr 9, 2024

@lpsinger if that works, that seems like a good idea to me. UFuncs currently do not have a __dict__ so unless we want to change that, we need to add the properties explicitly (which we can just do, though).
We should actually probably do the same for __module__ and maybe also __qualname__ for pickling and inspection (who knows, maybe also __signature__... maybe we should just add a __dict__!? Python is a language that generally is OK with allowing monkey patching after all).

The one caveat is that it might be better to not modify the size of the ufunc object in bug-fix releases (we may actually have some empty slots, though).

EDIT: Sorry for double post, github was misbehaving.

@WarrenWeckesser
Copy link
Member

... maybe we should just add a __dict__!?

FWIW, this is a feature I would like, but it was never a high enough priority to investigate an implementation. The use-case is a function where an integer parameter is effectively an enum, and I wanted to store the allowed values as attributes on the ufunc. See the fifth bullet of the list here: https://github.com/WarrenWeckesser/numpy-notes/blob/main/enhancements/gufunc-wishlist.md

@lpsinger
Copy link
Contributor Author

lpsinger commented Apr 9, 2024

Hmm, unfortunately ufunc_get_doc does not return ufunc->doc; it does some formatting to add the signature in order to compute the value to return for __doc__. So simply adding a setter to go with it would not make a lot of sense, because

>>> func.__doc__ = 'foo'
>>> assert func.__doc__ == 'foo'

would fail.

What about adding a ufunc.doc prop?

lpsinger added a commit to lpsinger/ligo.skymap that referenced this issue Apr 9, 2024
The funcion `add_newdoc_ufunc` is no longer part of the Numpy
public API, although according to upstream developers it is not
going away any time soon.

See numpy/numpy#26233.
@seberg
Copy link
Member

seberg commented Apr 10, 2024

So simply adding a setter to go with it would not make a lot of sense

Maybe a setter can work, though? You could give it a logic of:

@property
def __doc__(self):
    if self._doc_obj:
        return self._doc_obj

    # old logic.

@__doc__.setter
def __doc__(self, value):
    self._doc_obj = value
    # Or if need be, leak it into the doc string but store the info
    # that __doc__ was called in some way.

If we add the __dict__, I don't mind if we store it as a _ufunc_doc in the dict also (doc lookup doesn't need to be optimized).

@lpsinger
Copy link
Contributor Author

So simply adding a setter to go with it would not make a lot of sense

Maybe a setter can work, though? You could give it a logic of:

The problem is that the getter for __doc__ must return the concatenation of the computed signature text plus the library-provided ufunc->doc string, whereas the setter would only take the latter. We certainly could add a setter, but as I said it would not obey the following behavior of most properties:

>>> obj.prop = value
>>> assert obj.prop == value

Are you OK with that?

@seberg
Copy link
Member

seberg commented Apr 11, 2024

Ah, you are right. I thought we would ask you to include the signature in your doc string, but that isn't great.
There may be a simple solution, but I haven't tried around with it: We could try to define the __text_signature__ instead and always exclude the signature from the docs.
That seems very right, but not sure if there is some subtlety that makes this harder.

(There was some discussion about using __text_signature__ earlier, I think. @eric-wieser would you know if there is some caveat, or whether this was just never done.)

@rgommers
Copy link
Member

I haven't followed in detail, but just a quick note that there is recent discussion/work on __text_signature__ in CPython, see https://discuss.python.org/t/type-signatures-for-extension-modules-pep-draft/43914

@seberg
Copy link
Member

seberg commented Apr 15, 2024

Just to come back: if assignment to __doc__ is weird, I would say a _set_doc or so method should OK, with the understanding that we might replace it with a better way at some point...

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

6 participants