DEP: Deprecate NPY_CHAR #8948

juliantaylor · 2017-04-15T11:54:25Z

Do the deprecation that should have happened 7 releases ago.
Attempt to fix f2py but there are no tests for the NPY_CHAR code so it is most likely busted.
As f2pys usage of NPY_CHAR severely hinders further development by blocking addition of new dtypes without breaking ABI or baking more hacks into our existing ABI I say release it and fix as reports come in.

mhvk · 2017-04-15T16:12:53Z

I used f2py quite a bit, so tried to review, but realized I don't understand things well enough to be of much use. Might it be an idea to warn on the mailing list and specifically ask people to add simple test cases for string behaviour that they know works right now? (my own code only passed through numbers, so is of little use, sadly).

juliantaylor · 2017-04-15T16:25:13Z

strings were apparently never really well supported and the few stackoverflow entries I have found are full of confused users. It has never been updated from the numarray times.
So my guess there are probably not too many users out there, but still its a good idea to ping the list.

rgommers · 2017-04-15T21:02:26Z

thanks for picking this up Julian!

pearu · 2017-04-17T07:14:07Z

Fortran character strings and arrays are very common in Fortran codes. So, please do not assume that there are not many users who need to wrap such Fortran codes for Python. I have helped out number of users in private regarding wrapping strings and string arrays. The fundamental trick is to not assume that Fortran strings (mutable) can be mapped directly to Python strings (immutable), and back. I have found that the mutability of strings in different languages is the main source of confusion.

I agree that f2py can be improved regarding strings support and I have promised to do that years ago. However, this work takes some time but it is really hard for me to find couple of weeks to do the required work.

juliantaylor · 2017-04-17T07:16:59Z

we don't need full string supports now, we just need to remove the NPY_CHAR usage.
Can you check if this PR does the correct thing?

pearu · 2017-04-18T17:00:42Z

I checked this PR with a test case containing

character*5, dimension (10), intent (inout) :: A
!f2py intent(in,out) A

and the test script gives the same output as with NPY_CHAR enabled.
So, with this very limited test I can confirm that the PR does the correct thing.

juliantaylor · 2017-04-22T10:42:02Z

numpy/f2py/src/fortranobject.c

@@ -701,6 +706,15 @@ PyArrayObject* array_from_pyobj(const int type_num,
    }

    descr = PyArray_DescrFromType(type_num);
+    /* compatibility with NPY_CHAR */
+    if (type_num == NPY_STRING) {
+        descr = PyArray_DescrNew(descr);


one could use PyArray_DESCR_REPLACE but that decrefs and PyArray_DescrFromType does not incref basic types.
I assume that is actually a bug we just get away with it because we likely leak descriptors all over the place.

oh it does incref I am just to dumb to read

eric-wieser · 2017-04-22T10:47:24Z

numpy/core/src/multiarray/arraytypes.c.src

@@ -4435,6 +4436,13 @@ PyArray_DescrFromType(int type)
        return NULL;
    }
    else if ((type == NPY_CHAR) || (type == NPY_CHARLTR)) {


Puzzled by the distinction between CHAR and CHARLTR - and why this patch seems to translate CHAR into CHARLTR now

It triggers some old numarray compatibility code. It probably doesn't work as f2py might very well be the only remaining user.
But this patch doesn't change anything in this regard.

The CHAR_LTR refers to the dtype(...).char attribute, which is the only difference from the string type.

In [1]: dtype('c') == dtype('S1') Out[1]: True

charris · 2017-04-22T17:40:41Z

@juliantaylor Needs a release note.

charris · 2017-04-25T14:29:38Z

Also needs an entry in numpy/core/tests/test_deprecations.py.

charris · 2017-04-25T14:37:25Z

numpy/core/include/numpy/ndarraytypes.h


+#if !defined _NPY_NO_DEPRECATIONS && \


Would look better with the pseudo functional form defined(...).

charris · 2017-04-25T14:42:58Z

numpy/core/src/multiarray/arraytypes.c.src

@@ -4435,6 +4436,13 @@ PyArray_DescrFromType(int type)
        return NULL;
    }
    else if ((type == NPY_CHAR) || (type == NPY_CHARLTR)) {
+        if (type == NPY_CHAR) {
+            if (DEPRECATE("The NPY_CHAR type_num is deprecated. "
+                          "Please port your code to use "


Need comment with date and numpy version. Something like

/* 2017-04-25, 1.13 */

charris · 2017-04-25T14:44:53Z

numpy/core/src/multiarray/multiarray_tests.c.src

@@ -608,6 +609,13 @@ incref_elide_l(PyObject *dummy, PyObject *args)
    return res;
 }

+static PyObject *
+npy_char_deprecation(PyObject *dummy)
+{


Need a note as to why this is done this way. I assume to get a deprecation warning.

charris · 2017-04-25T14:49:10Z

numpy/f2py/cfuncs.py

    cppmacros[
-        m] = '#define %s(v,dims) (PyArray_SimpleNewFromData(1,dims,NPY_CHAR,(char *)v))' % (m)
+        m] = '#define %s(v,dims) (PyArray_New(&PyArray_Type, 1, dims, NPY_STRING, NULL, v, 1, NPY_ARRAY_CARRAY, NULL))' % (m)


Be good to break this string into shorter lines. Breaking the index bracket to get a continuation line is just weird, but that is for another day.

charris · 2017-04-25T14:51:07Z

numpy/f2py/src/fortranobject.c

@@ -679,6 +679,11 @@ PyArrayObject* array_from_pyobj(const int type_num,
        || ((intent & F2PY_OPTIONAL) && (obj==Py_None))
        ) {
        /* intent(cache), optional, intent(hide) */
+        int itemsize = 0;
+        /* compatibility with NPY_CHAR */


No zero sized strings the reason for 1?

EDIT: I am curious as to why it is set to 0 as 'c' is already 'S1'.

charris · 2017-04-25T15:27:10Z

numpy/f2py/src/fortranobject.c

+            return NULL;
+        }
+        descr->elsize = 1;
+        descr->type = NPY_CHARLTR;


Do we need to keep using NPY_CHARLTR? The only difference from S1 I can see is that that dtype('c').char is 'c'.

probably not, but that define doesn't really bother use in a non-cosmetic way.

charris · 2017-04-25T15:48:57Z

numpy/f2py/src/fortranobject.c

@@ -691,7 +696,7 @@ PyArrayObject* array_from_pyobj(const int type_num,
        }
        arr = (PyArrayObject *)
            PyArray_New(&PyArray_Type, rank, dims, type_num,
-                        NULL,NULL,0,
+                        NULL,NULL,itemsize,


I think you can just set this to 1. It is ignored for types of fixed size, which should be the case for NPY_CHAR and all the numeric types, so only used for string types

charris · 2017-04-25T15:50:40Z

numpy/f2py/src/fortranobject.c

                PyArray_New(&PyArray_Type, PyArray_NDIM(arr), PyArray_DIMS(arr), type_num,
-                            NULL,NULL,0,
+                            NULL,NULL,itemsize,


charris · 2017-04-25T15:52:48Z

numpy/core/tests/test_multiarray.py

@@ -342,6 +342,10 @@ def test_array_cont(self):
        assert_(np.ascontiguousarray(d).flags.c_contiguous)
        assert_(np.asfortranarray(d).flags.f_contiguous)

+    def test_npy_char_deprecation(self):


Belongs in test_deprecations.py

I can't get it to work the test_deprecation way.

class TestNPY_CHAR(_DeprecationTestCase): def test_npy_char_deprecation(self): from numpy.core.multiarray_tests import npy_char_deprecation self.assert_deprecated(npy_char_deprecation)

does not work, it says AssertionError: No error raised during function call. How is the test supposed to look like?

Its not really important to use that infrastructure anymore I think. That said, this may be correct, since the test checks both that the warning is given, as well as tests that an error is raised when the warning is set to "raise".

well, I guess this would work out with that test, if you just return the descriptor in the test function (since it should be NULL on error I guess).

ah that was it thanks. Updated.

charris · 2017-05-02T17:35:16Z

@juliantaylor Could you finish this up for 1.13?

[ci skip]

charris · 2017-05-03T17:00:47Z

Thanks Julian.

This avoids the following compilation message sherpa/include/sherpa/extension.hh:32:30: warning: ‘NPY_CHAR’ is deprecated: Use NPY_STRING [-Wdeprecated-declarations] and avoids a deprecation warning when running code in NumPy 1.13 and higher (see numpy/numpy#8948). The NPY_STRING symbol was added in NumPy 1.7, which I think is early enough that we don't need to try and support both the old and new versions (i.e. we do not guarantee support for Sherpa using versions as old as NumPy 1.7).

charris · 2017-11-15T17:00:42Z

@juliantaylor We finally have a report of an issue, #10027.

juliantaylor mentioned this pull request Apr 15, 2017

Deprecate NPY_CHAR #2801

Closed

juliantaylor force-pushed the deprecate-NPY_CHAR branch 2 times, most recently from f549803 to 34369b7 Compare April 15, 2017 12:54

rgommers added 07 - Deprecation component: numpy._core labels Apr 15, 2017

juliantaylor added this to the 1.13.0 release milestone Apr 22, 2017

juliantaylor commented Apr 22, 2017

View reviewed changes

juliantaylor force-pushed the deprecate-NPY_CHAR branch from fba23ca to 245a52d Compare April 22, 2017 10:44

eric-wieser reviewed Apr 22, 2017

View reviewed changes

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

charris changed the title ~~deprecate NPY_CHAR~~ DEP: Deprecate NPY_CHAR Apr 25, 2017

charris reviewed Apr 25, 2017

View reviewed changes

TST: add a char array input test

b8d0498

juliantaylor added 3 commits May 2, 2017 22:03

MAINT: remove usage of NPY_CHAR from f2py

a618b4e

MAINT: add deprecation warnings for NPY_CHAR usage

9c39207

DOC: add NPY_CHAR note to release notes

07dd1c9

[ci skip]

juliantaylor force-pushed the deprecate-NPY_CHAR branch from 245a52d to 9c39207 Compare May 3, 2017 00:19

MAINT: Add date and numpy version to TestNPY_CHAR.

7dbdbfa

charris merged commit 1d592c1 into numpy:master May 3, 2017

homu mentioned this pull request May 3, 2017

BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate) #9003

Closed

juliantaylor deleted the deprecate-NPY_CHAR branch May 4, 2017 18:07

DougBurke mentioned this pull request Aug 29, 2017

Remove unused typedef (avoid NPY_CHAR is deprecated warning) sherpa/sherpa#386

Merged

charris mentioned this pull request Nov 15, 2017

Issue with f2py in Numpy 1.13 when character arrays appear in a callback function #10027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP: Deprecate NPY_CHAR #8948

DEP: Deprecate NPY_CHAR #8948

juliantaylor commented Apr 15, 2017

mhvk commented Apr 15, 2017

juliantaylor commented Apr 15, 2017

rgommers commented Apr 15, 2017

pearu commented Apr 17, 2017

juliantaylor commented Apr 17, 2017

pearu commented Apr 18, 2017

juliantaylor Apr 22, 2017

juliantaylor Apr 22, 2017

eric-wieser Apr 22, 2017

juliantaylor Apr 22, 2017

charris Apr 25, 2017

charris commented Apr 22, 2017

charris commented Apr 25, 2017

charris Apr 25, 2017

charris Apr 25, 2017

charris Apr 25, 2017

charris Apr 25, 2017

charris Apr 25, 2017 •

edited

Loading

charris Apr 25, 2017

juliantaylor May 2, 2017

charris Apr 25, 2017

charris Apr 25, 2017

charris Apr 25, 2017

juliantaylor May 2, 2017

seberg May 2, 2017

seberg May 2, 2017

juliantaylor May 3, 2017

charris commented May 2, 2017

charris commented May 3, 2017

charris commented Nov 15, 2017

DEP: Deprecate NPY_CHAR #8948

DEP: Deprecate NPY_CHAR #8948

Conversation

juliantaylor commented Apr 15, 2017

mhvk commented Apr 15, 2017

juliantaylor commented Apr 15, 2017

rgommers commented Apr 15, 2017

pearu commented Apr 17, 2017

juliantaylor commented Apr 17, 2017

pearu commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Apr 22, 2017

charris commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented May 2, 2017

charris commented May 3, 2017

charris commented Nov 15, 2017

charris Apr 25, 2017 •

edited

Loading