Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Closed
wants to merge 1 commit into from

Conversation

ngoldbaum
Copy link
Member

Fixes #26233

Marked as draft because I have a couple questions:

  • Changing the C API isn't strictly necessary since there's a void *reserved2 field I can re-use for this. Any reason not to?
  • You can do this now:
In [1]: np.add.my_attribute = 7

In [2]: np.add.__dict__
Out[2]: {'my_attribute': 7}

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.

Copy link
Contributor

@mhvk mhvk left a 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
Copy link
Contributor

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) {
Copy link
Contributor

@mhvk mhvk Sep 13, 2024

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) {
Copy link
Contributor

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraneous extra space

@ngoldbaum
Copy link
Member Author

Closing in favor of #27735

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

Successfully merging this pull request may close these issues.

ENH: add __dict__ to the ufunc object
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.