MAINT: move comparison operator special-handling out of ufunc parsing. #11282

mhvk · 2018-06-08T15:29:57Z

The argument parsing for ufuncs contains special logic to enable returning NotImplemented for comparison functions. The ufuncs, however, should not have to know whether they were called via a python comparison operator, so this PR moves the logic to the operators, checking after ufunc failed
whether a NotImplemented should be returned.

Follow-up of #11260, in particular #11260 (comment)

mhvk · 2018-06-08T15:36:58Z

numpy/core/src/multiarray/arrayobject.c

+         * is counted as inflexble.  (This repeats work done in the ufunc,
+         * but OK to waste some time in an unlikely path.)
+         */
+        if (PyArray_GetArrayParamsFromObject(


I tried using the seemingly more logical PyArray_FromAny(other, NULL, 0, 0, 0, NULL) but somehow this returns a failure when other is a python string.

You probably need to PyErr_Fetch above, before you start calling any more functions - most python functions are not willing to run if an exception is actively being thrown

Ah, yes, that indeed does it!

eric-wieser · 2018-06-08T15:47:18Z

This presumably means np.equal("one", 1) no longer returns `NotImplemented?

mhvk · 2018-06-08T15:52:05Z

@eric-wieser - yes, indeed, the ufuncs themselves will no longer return NotImplemented - which means this would definitely need a note to that extent. So, not for 1.15!

eric-wieser · 2018-06-08T15:55:08Z

Can you add some tests for ufuncs no longer returning NotImplemented?

eric-wieser · 2018-06-08T15:56:37Z

numpy/core/src/multiarray/arrayobject.c

+ * silenced error.
+ */
+NPY_NO_EXPORT int
+DEPRECATE_FUTUREWARNING_silence_error(const char *msg) {


Maybe better to just add a PyObject *warning_type argument to the original function, and use PyErr_WarnEx(warning_type, msg, 1) in place of DEPRECATE_FUTUREWARNING

Since I needed the fetch the error anyway, the need for the new function disappeared...

eric-wieser · 2018-06-08T15:57:35Z

numpy/core/src/multiarray/arrayobject.c

+        int other_type_num, other_is_flexible;
+        /*
+         * Determine whether other has a flexible dtype; here, inconvertible
+         * is counted as inflexble.  (This repeats work done in the ufunc,


eric-wieser · 2018-06-08T15:59:48Z

numpy/core/src/multiarray/arrayobject.c

+            return Py_NotImplemented;
+        }
+    else if (other_is_flexible) {
+            /*


weird indent

mattip · 2018-06-08T16:33:34Z

It would be nice even at this early stage to document the user-visible changes and change the tests appropriately

mhvk · 2018-06-08T17:18:11Z

OK, now simpler with PyArray_FromAny - and some tests that were ready to go enabled.

@mattip - agreed about documentation, but there isn't even a file for the 1.16 release notes yet... But the main effect is the one @eric-wieser noted: now the ufuncs themselves will error rather than return NotImplemented (surely a good thing...)

charris · 2018-06-08T17:40:52Z

Just to emphasize, do not merge before the 1.15 branch.

mhvk · 2018-06-08T18:34:07Z

OK, tests now all pass - further review much appreciated!

mhvk · 2018-06-10T17:17:28Z

Rebased, so now this is just a single commit.

mhvk · 2018-06-14T17:41:21Z

With 1.15 branches, I rebased this and added a release note for 1.16. If everyone is still happy with this, it would be nice to merge it, since I have other changes to the ufunc parsing in the pipeline (#11333 in particular).

mhvk · 2018-06-18T19:36:08Z

@mattip, @eric-wieser - any further comments on this?

eric-wieser · 2018-06-18T19:50:27Z

numpy/core/src/multiarray/arrayobject.c

+            Py_DECREF(array_other);
+        }
+        else {
+            PyErr_Clear(); /* we restore the original error if needed */


Would be clearer if other_is_flexible and ndim_other were assigned in this path, rather than initialized.

The argument parsing for ufuncs contains special logic to enable returning NotImplemented for comparison functions. The ufuncs, however, should not have to know whether they were called via a python comparison operator, so this PR moves the logic to the operators, checking after ufunc failed whether a NotImplemented should be returned.

mhvk · 2018-06-18T20:11:36Z

OK, thanks, did all three.

eric-wieser · 2018-06-18T20:42:24Z

numpy/core/src/multiarray/arrayobject.c

+                        /*
+                         * In future, we should create a correctly shaped
+                         * array of bool. For now, a placeholder error.
+                         */


I think this should

Appear before the DEPRECATE_FUTUREWARNING line

say placeholder warning, not placeholder error.

It is just that if the deprecation is removed, the chained exception should be replaced (I wanted to have code that would at least work, but didn't feel like creating the array now...). I clarified this.

I'd say that's obvious, in the same way that the return NULL should also be replaced

eric-wieser · 2018-06-18T20:44:51Z

numpy/core/src/multiarray/arrayobject.c

+        return Py_NotImplemented;
+    }
+    if (result == NULL) {
+        /*


Can you extract the contents of this long if to _failed_compare_workaround(self, other, op), or something similar (documented to expect an exception to already be in flight)

mhvk · 2018-06-18T21:39:56Z

OK, pushed an extra commit moving the failure path to its own function.

mattip · 2018-06-18T21:50:18Z

numpy/core/src/multiarray/arrayobject.c

+         * ordering, so we emit a warning.
+         */
+#if !defined(NPY_PY3K)
+        if (DEPRECATE(


Date and version comment needed

mattip · 2018-06-18T21:50:33Z

numpy/core/src/multiarray/arrayobject.c

+             * If other did not have a flexible dtype, the error cannot
+             * have been caused by a lack of implementation in the ufunc.
+             */
+            if (DEPRECATE(


Date and version comment needed

mattip · 2018-06-18T21:51:37Z

numpy/core/src/multiarray/arrayobject.c

+             * array of bool.
+             */
+            if (ndim_other != 0 || PyArray_NDIM(self) != 0) {
+                if (DEPRECATE_FUTUREWARNING(


Date and version comment needed

eric-wieser · 2018-06-18T22:26:36Z

numpy/core/src/multiarray/arrayobject.c

+                    "elementwise comparison failed; "
+                    "this will raise an error in the future.") < 0) {
+                PyArray_ChainExceptionsCause(exc, val, tb);
+                return NULL;


I think it would be tidy to move this to a fail: label

eric-wieser · 2018-06-18T22:26:56Z

numpy/core/src/multiarray/arrayobject.c

+    }
+    else {
+        /* LE, LT, GT, or GE with non-flexible other; just pass on error */
+        PyErr_Restore(exc, val, tb);


This could also be the fail label - PyArray_ChainExceptionsCause is a shorhand for PyErr_Restore if no exception is set.

Yes, this makes a lot more sense.

mhvk · 2018-06-19T00:51:50Z

@mattip, @eric-wieser - all implemented.

eric-wieser · 2018-06-19T02:09:08Z

numpy/core/src/multiarray/arrayobject.c

+
+    /* LE, LT, GT, or GE with non-flexible other; just pass on error */
+fail:
+    PyErr_Restore(exc, val, tb);


This line should be the ChainExceptions, not PyErr_Restore. The former implies the latter, not vice versa

A comment here saying "Reraise the original exception, or chain the new one" might be nice too

eric-wieser · 2018-06-19T04:39:31Z

doc/release/1.16.0-notes.rst

+Comparison ufuncs will now error rather than return NotImplemented
+------------------------------------------------------------------
+
+Previously, comparison ufuncs would return `NotImplemented` if their arguments


"such as np.equal" might be helpful here.

eric-wieser · 2018-06-19T04:39:57Z

doc/release/1.16.0-notes.rst

+------------------------------------------------------------------
+
+Previously, comparison ufuncs would return `NotImplemented` if their arguments
+had structured dtypes, to help the comparison operators deal with those.  This


Can you point out that the comparison operators continue to return NotImplemented as before?

eric-wieser · 2018-06-19T04:41:57Z

numpy/core/src/multiarray/arrayobject.c

+             * For scalars, returning NotImplemented is correct.
+             * For arrays, we emit a future deprecation warning.
+             * If that is removed, the failure jump should be replaced
+             * by the creation of a correctly shaped array of bool.


Perhaps just "when this warning is removed, it should return a correctly-shaped array of bool.

eric-wieser · 2018-06-19T04:42:35Z

numpy/core/src/multiarray/arrayobject.c

+        return Py_NotImplemented;
+    }
+
+    /* LE, LT, GT, or GE with non-flexible other; just pass on error */


I'd probably add an else { goto fail; } here, just to have somewhere obvious to tie the comment to. The compiler should optimize it just the same anyway

mhvk · 2018-06-19T11:18:44Z

@eric-wieser - thanks for the further comments; all done.

eric-wieser

Implementation looks good now.

Did you just cut up the original long comment, or did you throw it out and reword it? I haven't yet checked to see if all of the important bits survived.

mhvk · 2018-06-20T13:10:50Z

Yes, I reworded the long comment, removing parts not relevant for comparison operators. I think it now (even) more clearly states where we want to go.

mhvk · 2018-06-21T12:55:07Z

As this was approved, I'll go ahead and merge so I can can rebase some of the other PRs (which will be easier to look at without the long distracting stuff in get_ufunc_arguments)

eric-wieser · 2018-06-21T16:40:44Z

numpy/core/src/multiarray/arrayobject.c

+    }
+    else {
+        /* LE, LT, GT, or GE with non-flexible other; just pass on error */
+        goto fail;


I'm not clear on why this path doesn't return NotImplemented in py3

The reason is that the comparison is actually implemented, i.e., the error is real and we act just like we would do for __add__ (note that the behaviour was the same before this PR).

mhvk requested review from eric-wieser and mattip June 8, 2018 15:29

mhvk commented Jun 8, 2018

View reviewed changes

mhvk requested a review from njsmith June 8, 2018 15:50

mhvk added component: numpy._core 03 - Maintenance 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels Jun 8, 2018

mhvk added this to the 1.16.0 release milestone Jun 8, 2018

eric-wieser reviewed Jun 8, 2018

View reviewed changes

numpy/core/src/multiarray/arrayobject.c Outdated

return Py_NotImplemented;

}

else if (other_is_flexible) {

/*

Copy link

Member

eric-wieser Jun 8, 2018

Choose a reason for hiding this comment

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

weird indent

mhvk force-pushed the array-comparison-not-implemented branch from c49d901 to c8dffb2 Compare June 8, 2018 17:15

mhvk mentioned this pull request Jun 8, 2018

MAINT: Do proper cleanup in get_ufunc_arguments. #11260

Merged

mhvk force-pushed the array-comparison-not-implemented branch from c8dffb2 to 61bd834 Compare June 8, 2018 18:06

mattip added the component: numpy.ufunc label Jun 8, 2018

mhvk force-pushed the array-comparison-not-implemented branch from 61bd834 to 72bab23 Compare June 10, 2018 17:16

mhvk force-pushed the array-comparison-not-implemented branch from 72bab23 to f7029ce Compare June 14, 2018 17:40

mhvk removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Jun 15, 2018

mhvk mentioned this pull request Jun 18, 2018

MAINT: Ufunc parsing super fast (work in progress) #11372

Closed

eric-wieser reviewed Jun 18, 2018

View reviewed changes

mhvk force-pushed the array-comparison-not-implemented branch from f7029ce to 13c42ef Compare June 18, 2018 20:11

eric-wieser reviewed Jun 18, 2018

View reviewed changes

mattip reviewed Jun 18, 2018

View reviewed changes

eric-wieser reviewed Jun 18, 2018

View reviewed changes

mhvk force-pushed the array-comparison-not-implemented branch from 8574044 to a2b9baa Compare June 19, 2018 00:51

eric-wieser reviewed Jun 19, 2018

View reviewed changes

mhvk added 2 commits June 19, 2018 07:17

DOC: add release note for change in output of comparison ufuncs.

4272dd0

MAINT: move failure code to its own function.

533bb37

mhvk force-pushed the array-comparison-not-implemented branch from a2b9baa to 533bb37 Compare June 19, 2018 11:17

eric-wieser reviewed Jun 20, 2018

View reviewed changes

mhvk merged commit 9c43b17 into numpy:master Jun 21, 2018

mhvk deleted the array-comparison-not-implemented branch June 21, 2018 12:55

eric-wieser reviewed Jun 21, 2018

View reviewed changes

This was referenced Jun 21, 2018

TST: DeprecationWarning elementwise comparison failed astropy/astropy#7587

Closed

BUG: Ensure comparisons on scalar strings pass without warning. #11405

Merged

Uh oh!

MAINT: move comparison operator special-handling out of ufunc parsing. #11282

MAINT: move comparison operator special-handling out of ufunc parsing. #11282

Uh oh!

Conversation

mhvk commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Jun 8, 2018

Uh oh!

mhvk commented Jun 8, 2018

Uh oh!

eric-wieser commented Jun 8, 2018

Uh oh!

eric-wieser Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Jun 8, 2018

Uh oh!

mhvk commented Jun 8, 2018

Uh oh!

charris commented Jun 8, 2018

Uh oh!

mhvk commented Jun 8, 2018

Uh oh!

mhvk commented Jun 10, 2018

Uh oh!

mhvk commented Jun 14, 2018

Uh oh!

mhvk commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

eric-wieser Jun 8, 2018 •

edited

Loading

eric-wieser Jun 18, 2018 •

edited

Loading

eric-wieser Jun 19, 2018 •

edited

Loading