MAINT: Deprecated PyObject_Compare in favor of PyObject_RichCompareBool. Fixes #6265 by yashmehrotra · Pull Request #6269 · numpy/numpy

yashmehrotra · 2015-08-29T20:22:46Z

@jaimefrio I made the changes as discussed in #6265 . I ran all tests for Python2 and Python3 through python runtests.py / python3 runtests.py and did not encounter any failure.

As far as the PyObject_Cmp function in npy_3kcompat.h goes, I did not change that. I although did fix the only part of code base that uses it with PyObject_RichCompareBool.
As far as PyObject_Cmp goes, should I remove it or add a deprecation warning ?

jaimefrio · 2015-08-29T21:35:30Z

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

Either one or minus_one is not going to be returned, and should therefore have its refcount decreased before returning, similarly to what happens with zero.

So should I decrease both one and minus_one's refcount in the end ?
Like

if (is_gt == 1) { ret = one; } else if (is_eq == 1) { ret = zero; } else { ret = minus_one; } if (PyErr_Occurred()) { Py_DECREF(zero); return; } Py_XDECREF(*out); *out = ret; } Py_DECREF(zero); Py_DECREF(one); Py_DECREF(minus_one);

All except the one you assign to out.

jaimefrio · 2015-08-29T21:47:18Z

As commented inline, fixing this will also close #6229, so please add a "closes #6229" somewhere in the commit message. And if you could also add a test for that failure it would be perfect.

yashmehrotra · 2015-08-29T21:56:21Z

@jaimefrio I didn't quite understand the test part, would I be checking for #6229 problem in the test case ?

yashmehrotra · 2015-08-29T23:36:10Z

@jaimefrio For reference count, I removed the one and minus_one as they were more of noise than signal. For issue #6229 , I did add the fix and the test case, although there is not much use of the fix as PyObject_Cmp is now not being used anywhere.
Are there any other changes I need to make to make this PR merge-ready ?

jaimefrio · 2015-08-30T04:18:42Z

The body of the OBJECT_sign function should be something like this:

PyObject *zero = PyLong_FromLong(0);

UNARY_LOOP {
    PyObject *in1 = *(PyObject **)ip1;
    PyObject **out = (PyObject **)op1;
    PyObject *ret = NULL;
    int v;

    if (in1 == NULL) {
        in1 = Py_None;
    }
    v = PyObject_RichCompareBool(in1, zero, Py_LT);
    if (v == 1) {
        ret = PyLong_FromLong(-1);
    }
    else if (v == 0) {
        v = PyObject_RichCompareBool(in1, zero, Py_GT);
        if (v == 1) {
            ret = PyLong_FromLong(1);
        }
        else if (v == 0) {
            v = PyObject_RichCompareBool(in1, zero, Py_EQ);
            if (v == 1) {
                ret = PyLong_FromLong(0);
            }
        }
    }
    if (v < 0) {
        Py_DECREF(zero);
        return;
    }
    Py_XDECREF(*out);
    *out = ret;
}
Py_DECREF(zero);
}

Feel free to rewrite it as you see fit, but beware of the subtleties: if we cannot get a positive comparison using Py_LT, Py_GT or Py_EQ, it is considered an error and we bail out early.

jaimefrio · 2015-08-30T04:19:49Z

numpy/core/include/numpy/npy_3kcompat.h

There are two more instances of this mistake, right after the next two calls to PyObject_RichCompareBool below.

yashmehrotra · 2015-08-30T06:41:01Z

@jaimefrio I have the changes as suggested by you. Thanks for the help. Are there any other things I need to do ?

jaimefrio · 2015-08-31T03:04:29Z

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

Extra space inside parenthesis.

yashmehrotra · 2015-08-31T03:17:11Z

@jaimefrio Extra space around parenthesis removed.

jaimefrio · 2015-08-31T03:23:18Z

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

I think there is a loophole in this logic. Consider the following:

>>> a = np.array([np.nan], dtype=object) >>> a < 0 array([False], dtype=bool) >>> a == 0 array([False], dtype=bool) >>> a > 0 array([False], dtype=bool)

If we have an object that is neither larger, nor smaller, nor equal to 0, right now we will set the output to NULL, which I am not sure is what we really want. Perhaps None is the right thing to set it to, but I'll ask on the list for the community wisdom on this one.

@jaimefrio I read a bit about NaN. In my opinion, comparing integers with NaN should return False as the comparison operators result is a boolean, and the comparison wouldn't be True.
Also, in JavaScript, all integer comparisons with NaN evaluates to false.
Well, that was my opinion. If it is decided to change the result to None as per the community decision,do keep updated on this one.

yashmehrotra · 2015-09-02T12:40:56Z

@jaimefrio Hi Jaime, do you have any updates for this PR ?

jaimefrio · 2015-09-04T06:02:53Z

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

Ok, so according to what seems to be the closest to a consensus, here we should have an else if (v == 0) that raises a TypeError, not sure what the right message should be. Thoughts?

@jaimefrio I have already implemented the above and its working as expected. For the error message, I was gonna ask you, what it should be 😛 .

How about we go with the error message you suggested in the mailing list or how about we just explicitly tell the user that the exception is due to NaN. Like TypeError: Trying to compare NaN (Something like this).

@jaimefrio Any thoughts on the error message or should I send a PR with error message TypeError: unorderable types ...etc

yashmehrotra · 2015-09-08T07:55:02Z

@jaimefrio Any feedback for the error message ?

yashmehrotra · 2015-09-10T13:47:39Z

@jaimefrio Can you please take a look at the new commit ?

yashmehrotra · 2015-09-15T05:10:09Z

@charris Hi Charles, it seems that Jaime is busy these days and I couldn't get any feedback on this PR. Can you please take a look at this.

yashmehrotra · 2015-09-25T09:48:16Z

@charris @jaimefrio How about merging this PR, its been pending for days. Is there any work required by my side to make it merge ready ?

ahaldane · 2015-09-25T13:40:02Z

Just to note, there is a possibility of overhauling OBJECT_sign in #6320 in the future. (But it may take a long time since that's waiting on __numpy_ufunc__ decisions). The implementation there takes care of complex numbers and float/complex nan more carefully.

However, this PR also has good cleanups for OBJECT_compare and _equivalent_fields (which there are no other plans to change), and I think the upgrade to OBJECT_sign in this PR is good to have in the short term.

charris · 2015-09-25T19:54:13Z

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

I'd be inclined to do this like

v = PyObject_RichCompareBool(in1, zero, Py_LT); if (v == 1) { ret = PyLong_FromLong(-1); goto done; } else if (v == -1) { goto error; }

and repeat with minor variations for the other two cases. That avoids the deep indentations and is, IMHO, easier to follow.
Note the style for else.

yashmehrotra · 2015-09-26T06:50:44Z

@charris I have made the changes as per your suggestions.

charris · 2015-09-27T00:47:15Z

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

Note style guide doc/C_STYLE_GUIDE.rst.txt. The else should be like

} else if {

yashmehrotra · 2015-09-27T02:23:22Z

@charris I have modified the code as required. I hope its all good now.

charris · 2015-09-27T22:54:04Z

I apologize for getting a bit confused myself. I'll take another look later this evening.

charris · 2015-09-28T02:37:44Z

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

Wrong endentation.

charris · 2015-09-28T02:42:14Z

LGTM modulo nitpick. Another possibility that adds no net lines and get rid of the goto is to replace

            ret = PyLong_FromLong(0);
            goto done;

by

            Py_XDECREF(*out)
            *out = PyLong_FromLong(0);
            continue;

etc.

yashmehrotra · 2015-09-28T04:07:44Z

@charris Changes updated. I hope its good now.

charris · 2015-09-28T15:59:13Z

OK, my bad. The other errors need checking. How about

NPY_NO_EXPORT void
OBJECT_sign(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED(func))
{
    PyObject *zero = PyLong_FromLong(0);

    UNARY_LOOP {
        PyObject *in1 = *(PyObject **)ip1;
        PyObject **out = (PyObject **)op1;
        PyObject *ret = NULL;
        int v = 0;

        if (in1 == NULL) {
            in1 = Py_None;
        }

        if ((v = PyObject_RichCompareBool(in1, zero, Py_LT)) == 1) {
            ret = PyLong_FromLong(-1);
        }
        else if (v == 0 &&
                (v = PyObject_RichCompareBool(in1, zero, Py_GT)) == 1) {
            ret = PyLong_FromLong(1);
        }
        else if (v == 0 &&
                (v = PyObject_RichCompareBool(in1, zero, Py_EQ)) == 1) {
            ret = PyLong_FromLong(0);
        }
        else if (v == 0) {
            /* in1 is NaN */
            PyErr_SetString(PyExc_TypeError,
                    "unorderable types for comparison");
        }

        if (ret == NULL) {
            break;
        }
        Py_XDECREF(*out);
        *out = ret;
    }
    Py_DECREF(zero);
}

yashmehrotra · 2015-09-28T19:46:20Z

@charris I have updated the code. Hope its all good to go now.

charris · 2015-09-28T19:53:43Z

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

Don't need this line.

charris · 2015-09-28T20:09:10Z

Almost there. When you are ready, squash the commits using git rebase -i HEAD~n, where n is the number of commits in this PR, then do a force push git push --force origin HEAD.

…ol. Fixes #6265 and #6229

yashmehrotra · 2015-09-28T20:16:58Z

@charris All commits squashed into one.

charris · 2015-09-28T20:25:21Z

LGTM pending tests.

MAINT: Deprecated PyObject_Compare in favor of PyObject_RichCompareBool. Fixes #6265

charris · 2015-09-28T20:48:16Z

Thanks @yashmehrotra . If you would like to make a PR against the maintenance/1.10.x branch that just fixes PyObject_Cmp and add tests for same I would appreciate it. If not, I will do it later today.

charris · 2015-09-29T00:07:57Z

I've backported the PyObject_Cmp fix, so don't do that.

jaimefrio reviewed Aug 29, 2015
View reviewed changes

jaimefrio reviewed Aug 30, 2015
View reviewed changes

jaimefrio reviewed Aug 31, 2015
View reviewed changes

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

Copy link

Member

jaimefrio Aug 31, 2015

Choose a reason for hiding this comment

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

Extra space inside parenthesis.

jaimefrio reviewed Aug 31, 2015
View reviewed changes

jaimefrio reviewed Sep 4, 2015
View reviewed changes

charris added component: numpy._core 03 - Maintenance labels Sep 21, 2015

charris reviewed Sep 25, 2015
View reviewed changes

charris reviewed Sep 27, 2015
View reviewed changes

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

Copy link

Member

charris Sep 27, 2015

Choose a reason for hiding this comment

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

Note style guide doc/C_STYLE_GUIDE.rst.txt. The else should be like

} else if {

charris reviewed Sep 28, 2015
View reviewed changes

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

Copy link

Member

charris Sep 28, 2015

Choose a reason for hiding this comment

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

Wrong endentation.

charris reviewed Sep 28, 2015
View reviewed changes

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

Copy link

Member

charris Sep 28, 2015

Choose a reason for hiding this comment

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

Don't need this line.

MAINT: Deprecated PyObject_Compare in favor of PyObject_RichCompareBo…

00edb2b

…ol. Fixes #6265 and #6229

charris added a commit that referenced this pull request Sep 28, 2015

Merge pull request #6269 from yashmehrotra/depr-pyobject_compare

f43d691

MAINT: Deprecated PyObject_Compare in favor of PyObject_RichCompareBool. Fixes #6265

charris merged commit f43d691 into numpy:master Sep 28, 2015

This was referenced Sep 28, 2015

MAINT: struct assignment "by field position", multi-field indices return views #6053

Merged

[ENH](WIP) new ufunc implementations for object arrays #6320

Closed

ahaldane mentioned this pull request Feb 20, 2016

BUG: Segfault when np.unique with bad array #7291

Closed

This was referenced Feb 20, 2016

numpy.sign behaves oddly when dtype is numpy.object #6229

Closed

BUG: object dtype quicksort can segfault #5961

Closed

Uh oh!

Conversation

yashmehrotra commented Aug 29, 2015

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!

jaimefrio commented Aug 29, 2015

Uh oh!

yashmehrotra commented Aug 29, 2015

Uh oh!

yashmehrotra commented Aug 29, 2015

Uh oh!

jaimefrio commented Aug 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmehrotra commented Aug 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmehrotra commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmehrotra commented Sep 2, 2015

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!

yashmehrotra commented Sep 8, 2015

Uh oh!

yashmehrotra commented Sep 10, 2015

Uh oh!

yashmehrotra commented Sep 15, 2015

Uh oh!

yashmehrotra commented Sep 25, 2015

Uh oh!

ahaldane commented Sep 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmehrotra commented Sep 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmehrotra commented Sep 27, 2015

Uh oh!

charris commented Sep 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Sep 28, 2015

Uh oh!

yashmehrotra commented Sep 28, 2015

Uh oh!

charris commented Sep 28, 2015

Uh oh!

yashmehrotra commented Sep 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Sep 28, 2015

Uh oh!

yashmehrotra commented Sep 28, 2015

Uh oh!

charris commented Sep 28, 2015

Uh oh!

charris commented Sep 28, 2015

Uh oh!

charris commented Sep 29, 2015

Uh oh!