-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
add a __dict__ to ufunc objects and use it to allow overriding __doc__ #27383
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we allow setting __doc__
, we should do that in all cases, i.e., also to override anything passed in originally. See also in-line comment.
Separately, agreed that it would make sense to use the reserved2
slot.
with pytest.raises(AttributeError, match=msg): | ||
np.add.__doc__ = "hello" | ||
|
||
# you can add a "__doc__" key to __dict__ but it doesn't do anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems not right...
@@ -6411,6 +6412,19 @@ ufunc_get_doc(PyUFuncObject *ufunc, void *NPY_UNUSED(ignored)) | ||
{ | ||
PyObject *doc; | ||
|
||
// if there is a __doc__ in the instance __dict__, use that, but only if | ||
// __doc__ is NULL | ||
if (ufunc->doc == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might reverse the logic: if __doc__
is present in ufunc->dict
return that. If not, use ufunc->doc
as now to calculate the final docstring, and then store the result in __doc__
(so that it gets re-used any further times, unless it is overwritten; and if one does del ufunc.__doc__
, one will automatically get the original docstring back).
EDIT: actually, storing the result of the normal calculation is not really necessary - it just uses extra space that can otherwise be freed.
static int | ||
ufunc_set_doc(PyUFuncObject *ufunc, PyObject *doc, void *NPY_UNUSED(ignored)) | ||
{ | ||
if (ufunc->doc != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my suggestion, this check would not be necessary.
@@ -6564,10 +6598,16 @@ NPY_NO_EXPORT PyTypeObject PyUFunc_Type = { | ||
.tp_str = (reprfunc)ufunc_repr, | ||
.tp_flags = Py_TPFLAGS_DEFAULT | | ||
_Py_TPFLAGS_HAVE_VECTORCALL | | ||
Py_TPFLAGS_HAVE_GC, | ||
Py_TPFLAGS_HAVE_GC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous extra space
Closing in favor of #27735 |
Fixes #26233
Marked as draft because I have a couple questions:
void *reserved2
field I can re-use for this. Any reason not to?Is that a thing we want? This adds a new way to monkeypatch numpy. People probably already do that in other pure python bits, but now they can do it with ufuncs too. I guess the consenting adults principle applies though and I'd be inclined to allow it.