ENH: add np.positive ufunc #8967

shoyer · 2017-04-21T07:23:58Z

The strategy here was to search for every use of "negative" in NumPy's C code, so I quite possibly missed something. But it builds, and seems to work.

Still needs full test coverage. The existing test coverage for ufuncs like absolute and negative is rather sparse, but that's no excuse for new code.

Questions to resolve:

Is there a better way to do this, perhaps still making use of PyArray_Copy?
Do I need to write an simd loop for floats? This looked a little tricky, so I didn't bother. But it does mean that positive is slightly slower than negative for floats (I measured 7% for a large array), which is a little strange.
Do we need to deprecate the existing behavior for the unary op instead of suddenly raising for operations like +np.array('foo')? How do we even detect these cases? I guess we can check for dtypes that are no longer valid (e.g., strings), but there's no way to know if unary + is a valid on an dtype=object array without trying it on each of the scalars.
Should + on a boolean array be valid? I guess the answer is no, given that -x raises for boolean arrays and based on the principle that +x should only be valid when -x is. This strengths the case for a deprecation cycle for the behavior of ndarray.__pos__, given that we went through such a deprecation cycle for negative on booleans (DEP: Handle expired deprecations. #8297).

shoyer · 2017-04-21T07:47:35Z

For now, it might be better to skip trying to fix up ndarray.__pos__, and only add the new ufunc for use with NDArrayOperatorsMixin.

eric-wieser · 2017-04-21T09:06:28Z

numpy/core/src/umath/loops.c.src

@@ -874,6 +874,14 @@ NPY_NO_EXPORT NPY_GCC_OPT_3 @ATTR@ void

 #if @CHK@
 NPY_NO_EXPORT NPY_GCC_OPT_3 @ATTR@ void
+@TYPE@_positive@isa@(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED(func))
+{
+    UNARY_LOOP_FAST(@type@, @type@, *out = in);


Can we do *out = +in here (and below), for clarity?

Then you haven't pushed!

eric-wieser · 2017-04-21T09:07:54Z

numpy/core/src/umath/loops.c.src

+    UNARY_LOOP {
+        const npy_half in1 = *(npy_half *)ip1;
+        *((npy_half *)op1) = in1;
+    }


I realize that you're just copying negative here - but what's the heuristic for when to use UNARY_LOOP_FAST and when to just use UNARY_LOOP? @juliantaylor, I suspect you can shed some light here?

you use UNARY_LOOP_FAST in cases where compiler vectorization is profitable.
It involves several copies of the loop so it is just bloat in cases where no vectorization can be done like in the negative half case.
You could use a fast loop for positive here (though it is hardly worth the extra generated code)
In general you could use a memcpy special case instead of the fast loop here too, like the logical_not in gh-8924. Some compilers might not be able to do that for you.

juliantaylor · 2017-04-21T10:55:02Z

numpy/core/src/multiarray/number.c

@@ -1128,7 +1139,7 @@ NPY_NO_EXPORT PyNumberMethods array_as_number = {
    (binaryfunc)array_divmod,                   /*nb_divmod*/
    (ternaryfunc)array_power,                   /*nb_power*/
    (unaryfunc)array_negative,                  /*nb_neg*/
-    (unaryfunc)_array_copy_nice,                /*nb_pos*/


this function can be removed now

With some regret, I decided to undo this change, because of the potential for a backwards compatibility break (we're pretty paranoid about those).

shoyer · 2017-04-26T01:47:49Z

Any further concerns here?

Note that I rolled back the changes to ndarray.__pos__

eric-wieser · 2017-04-26T08:24:37Z

Ah, but needs a release note

eric-wieser · 2017-04-26T18:56:52Z

Proposal - can all the positive loops be combined into the same template as the negative loops? So add a repeat with @op = +, -@, @func = positive, negative@. Just in the interest of keeping what is an already large file to a somewhat reasonable size.

charris · 2017-04-30T17:19:17Z

@shoyer Needs a rebase and release note.

homu · 2017-04-30T17:28:51Z

☔ The latest upstream changes (presumably #8885) made this pull request unmergeable. Please resolve the merge conflicts.

xref GH8932

shoyer · 2017-05-01T03:45:34Z

Proposal - can all the positive loops be combined into the same template as the negative loops? So add a repeat with @op = +, -@, @func = positive, negative@. Just in the interest of keeping what is an already large file to a somewhat reasonable size.

This is a good idea, but it turned out to be easier said that done:

For integers, negative uses a non-trivial SIMD trick
For timedelta, negative needs to check for NaT
For floats, negative also uses SIMD
For half, negative also uses something clever

So I just stuck with separate definitions.

Needs a rebase and release note

Done and done.

eric-wieser · 2017-05-01T09:28:20Z

Thanks @shoyer!

mhvk · 2017-05-08T18:46:29Z

I'm puzzled, this one function does not seem to work well with __array_ufunc__:

class A(np.ndarray):
    def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
        return "A"

a = np.array(1.).view(A)
-a
# 'A'
+a
# 1.0

I think it is due to positive still being defined as _array_copy_nice in number.c, but simply changing that and adding to GET and SET seemed insufficient:

numpy/core/src/multiarray/number.c:573:50: error: ‘NumericOps {aka struct <anonymous>}’ has no member named ‘positive’; did you mean ‘negative’?
     return PyArray_GenericUnaryFunction(m1, n_ops.positive);

(will try to investigate later, but have to run now; if someone else understands what is happening, do go ahead and fix...)

eric-wieser · 2017-05-08T18:47:59Z

@shoyer deliberately did not connect up np.positive to __pos__ yet, so I think this is expected behaviour?

mhvk · 2017-05-08T19:32:35Z

OK, rereading the comments I see that is the case. I guess I'll have to keep Quantity.__pos__ for another release then...

shoyer · 2017-05-08T19:52:53Z

@mhvk if you think it would be OK from a compatibility perspective, we could switch ndarray.__pos__. But again, I am concerned that this might cause a minor compatibility break.

mhvk · 2017-05-08T19:56:20Z

Probably being careful is good, but should we at least add the deprecation warning? Also, maybe just pass on to np.positive for classes that define __array_ufunc__? (Since those obviously know what is happening.)

shoyer · 2017-05-08T19:59:37Z

@mhvk Those both sound like fine ideas to me.

mhvk · 2017-05-08T20:02:26Z

@shoyer - would you be able to do it? Or have an idea what I should do beyond what I tried (#8967 (comment))

shoyer · 2017-05-09T19:19:11Z

@mhvk I'm not sure I'll have time to get to this before the branch for 1.13. Probably, the desired behavior is something like:

def __pos__(self):
    try:
        return np.positive(self)
    except TypeError:
        issue deprecation warning
        return self.copy()

(This may be a little tricky, though, because it will need to be done in C.)

For your ndarray subclass, you could overwrite __pos__ to call np.positive, though would probably need a fallback option for older versions of numpy where np.positive is not available.

mhvk · 2017-05-09T19:24:01Z

The override is indeed what I do for Quantity; I'm also not sure I'll have time for another crack at this.

shoyer · 2017-05-09T19:37:45Z

I created #9081 so we don't forget about this.

shoyer mentioned this pull request Apr 21, 2017

Missing ufuncs: np.divmod and np.positive #8932

Closed

eric-wieser reviewed Apr 21, 2017

View reviewed changes

juliantaylor reviewed Apr 21, 2017

View reviewed changes

eric-wieser added 01 - Enhancement component: numpy._core labels Apr 21, 2017

shoyer changed the title ~~ENH: add np.positive ufunc and use it for ndarray.__pos__~~ ENH: add np.positive ufunc Apr 22, 2017

eric-wieser added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Apr 22, 2017

shoyer force-pushed the positive branch from e324aa6 to 4168214 Compare April 22, 2017 23:31

eric-wieser approved these changes Apr 26, 2017

View reviewed changes

shoyer added this to the 1.13.0 release milestone Apr 27, 2017

ENH: add np.positive ufunc and use it for ndarray.__pos__

8db9662

xref GH8932

shoyer force-pushed the positive branch from 8cce713 to 8db9662 Compare May 1, 2017 03:41

ENH: add __pos__ to NDArrayOperatorsMixin

e799be5

shoyer removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 1, 2017

eric-wieser merged commit d46df62 into numpy:master May 1, 2017

This was referenced May 1, 2017

ENH: implement voidtype_repr and voidtype_str #8981

Merged

ENH: Make isfinite work with datetime/timedelta. #7856

Closed

ENH: Print object arrays containing lists unambiguously #8989

Merged

shoyer deleted the positive branch May 8, 2017 19:51

shoyer mentioned this pull request May 9, 2017

MAINT, DOC: Update 1.13.0 release notes and .mailmap #9080

Merged

shoyer mentioned this pull request May 9, 2017

ENH: call np.positive in ndarray.__pos__, and issue a deprecation warning when it falls #9081

Closed

asi1024 mentioned this pull request Apr 8, 2021

Support __pos__ and positive pytorch/pytorch#55604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add np.positive ufunc #8967

ENH: add np.positive ufunc #8967

shoyer commented Apr 21, 2017 •

edited

Loading

shoyer commented Apr 21, 2017

eric-wieser Apr 21, 2017 •

edited

Loading

shoyer Apr 22, 2017

eric-wieser Apr 22, 2017

eric-wieser Apr 21, 2017

juliantaylor Apr 21, 2017

juliantaylor Apr 21, 2017

shoyer Apr 22, 2017

shoyer commented Apr 26, 2017

eric-wieser commented Apr 26, 2017

eric-wieser commented Apr 26, 2017 •

edited

Loading

charris commented Apr 30, 2017

homu commented Apr 30, 2017

shoyer commented May 1, 2017 •

edited

Loading

eric-wieser commented May 1, 2017

mhvk commented May 8, 2017

eric-wieser commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 9, 2017

mhvk commented May 9, 2017

shoyer commented May 9, 2017

ENH: add np.positive ufunc #8967

ENH: add np.positive ufunc #8967

Conversation

shoyer commented Apr 21, 2017 • edited Loading

shoyer commented Apr 21, 2017

eric-wieser Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

shoyer Apr 22, 2017

Choose a reason for hiding this comment

eric-wieser Apr 22, 2017

Choose a reason for hiding this comment

eric-wieser Apr 21, 2017

Choose a reason for hiding this comment

juliantaylor Apr 21, 2017

Choose a reason for hiding this comment

juliantaylor Apr 21, 2017

Choose a reason for hiding this comment

shoyer Apr 22, 2017

Choose a reason for hiding this comment

shoyer commented Apr 26, 2017

eric-wieser commented Apr 26, 2017

eric-wieser commented Apr 26, 2017 • edited Loading

charris commented Apr 30, 2017

homu commented Apr 30, 2017

shoyer commented May 1, 2017 • edited Loading

eric-wieser commented May 1, 2017

mhvk commented May 8, 2017

eric-wieser commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 8, 2017

mhvk commented May 8, 2017

shoyer commented May 9, 2017

mhvk commented May 9, 2017

shoyer commented May 9, 2017

shoyer commented Apr 21, 2017 •

edited

Loading

eric-wieser Apr 21, 2017 •

edited

Loading

eric-wieser commented Apr 26, 2017 •

edited

Loading

shoyer commented May 1, 2017 •

edited

Loading