ENH: Allow ufunc.identity to be any python object #8955
Conversation
| {nin}, {nout}, {identity}, "{name}", | ||
| "{doc}", 0, NULL, identity | ||
| ); | ||
| if (f == NULL) {{ |
There was a problem hiding this comment.
Previously we never did any error checking here, which seems wrong
| docstrings.get('numpy.core.umath.logical_or'), | ||
| 'PyUFunc_SimpleBinaryComparisonTypeResolver', | ||
| TD(nodatetime_or_obj, out='?', simd=[('avx2', ints)]), | ||
| TD(O, f='npy_ObjectLogicalOr'), | ||
| ), | ||
| 'logical_xor': | ||
| Ufunc(2, 1, Zero, | ||
| Ufunc(2, 1, False_, |
There was a problem hiding this comment.
Zero wasn't really correct, but there was previously no way to specify False
52afba2 to
97e36ad
Compare
|
☔ The latest upstream changes (presumably #8876) made this pull request unmergeable. Please resolve the merge conflicts. |
907dc3e to
8cea62b
Compare
|
Needs rebase. There seem to be several PRs in this series that should go in together. |
8cea62b to
e180357
Compare
|
Updated |
e180357 to
27d9f1a
Compare
|
I would suggest adding identities to xref #5032 |
|
@hameerabbasi: I think that's a bad idea (think integer types, |
|
(base changed to make github recompute the diff) |
|
From https://github.com/numpy/numpy/pull/10635/files#r170466944 - once #10635 is merged, we should be sure to correct the code such that the initializer is also used as one of the elements over which reductions are done for object dtype (in #10635, it is only used for empty slices). |
|
This might make sense to get into 1.16, while we're adding members to ufuncs |
|
Indeed, I can review again if rebased. |
27d9f1a to
aa50061
Compare
|
Rebased, with some more docs |
aa50061 to
e475e76
Compare
doc/source/reference/c-api.ufunc.rst
Outdated
| The identity for the new gufunc. Must be passed as ``NULL`` unless the | ||
| ``identity`` argument is ``PyUFunc_IdentityValue``. Setting it to NULL | ||
| is equivalent to calling PyUFunc_FromFuncAndDataAndSignature. | ||
| The reference is stolen. |
There was a problem hiding this comment.
I'm not really experienced enough with C apis to know whether this is a good idea - does CPython provide any guidelines on when to steal and not to steal references?
There was a problem hiding this comment.
Generally, it seems to be stolen only when the reference can reasonably expected to be stored elsewhere (as in setting an element of a list). That does seem to be the case here. That said, the case is less clear than for a list, and the documentation [1] explicitly states that reference stealing functions are exceptions. So, my sense would be not to steal the reference.
[1] https://docs.python.org/3/c-api/intro.html#reference-count-details
doc/source/reference/c-api.ufunc.rst
Outdated
| The identity for the new gufunc. Must be passed as ``NULL`` unless the | ||
| ``identity`` argument is ``PyUFunc_IdentityValue``. Setting it to NULL | ||
| is equivalent to calling PyUFunc_FromFuncAndDataAndSignature. | ||
| The reference is stolen. |
There was a problem hiding this comment.
Generally, it seems to be stolen only when the reference can reasonably expected to be stored elsewhere (as in setting an element of a list). That does seem to be the case here. That said, the case is less clear than for a list, and the documentation [1] explicitly states that reference stealing functions are exceptions. So, my sense would be not to steal the reference.
[1] https://docs.python.org/3/c-api/intro.html#reference-count-details
08204dd to
77347a3
Compare
| @@ -685,6 +685,10 @@ def test_nan(self): | |||
| assert_(np.isnan(np.logaddexp(0, np.nan))) | |||
| assert_(np.isnan(np.logaddexp(np.nan, np.nan))) | |||
|
|
|||
| def test_reduce(self): | |||
| assert_equal(np.logaddexp.identity, -np.inf) | |||
| assert_equal(np.logaddexp.reduce([]), -np.inf) | |||
There was a problem hiding this comment.
@mhvk: Updated with a slightly longer testcase
|
Note: This will need a rebase if #11977 goes in first, even though github won't show conflicts. |
|
Shall we merge now to avoid the need for a rebase? I think this is all OK. |
|
Yeah, why not. Updated with release notes. |
| @@ -402,6 +402,9 @@ | |||
| # End 1.7 API | |||
| 'PyUFunc_RegisterLoopForDescr': (41,), | |||
| # End 1.8 API | |||
| 'PyUFunc_FromFuncAndDataAndSignatureAndIdentity': | |||
| (42,) | |||
| # End 1.15 API | |||
42d1fe2 to
e044ae3
Compare
|
Thanks Eric |
The lack of identity for `logaddexp2` was first identitifed in numpy#4599. The implementation in numpy#8955 added -inf as identity for `logaddexp`, but missed adding it for `logaddexp2`.
The lack of identity for `logaddexp2` was first identitifed in numpy#4599. The implementation in numpy#8955 added -inf as identity for `logaddexp`, but missed adding it for `logaddexp2`.
The first commit in this PR is #8952, which probably ought to be merged first(done)Fixes #7702 and #4599, and also a related issue to #8860