DEP: Deprecate NPY_CHAR by juliantaylor · Pull Request #8948 · numpy/numpy

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

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

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

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

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

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

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

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

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

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

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

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

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

numpy/core/tests/test_multiarray.py

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

numpy/core/include/numpy/ndarraytypes.h Outdated

Copy link
Copy Markdown

Member

charris Apr 25, 2017

Choose a reason for hiding this comment

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

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

charris reviewed Apr 25, 2017

View reviewed changes

numpy/f2py/src/fortranobject.c Outdated

Copy link
Copy Markdown

Member

charris Apr 25, 2017

Choose a reason for hiding this comment

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

See above.

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

Uh oh!

Conversation

juliantaylor commented Apr 15, 2017

Uh oh!

mhvk commented Apr 15, 2017

Uh oh!

juliantaylor commented Apr 15, 2017

Uh oh!

rgommers commented Apr 15, 2017

Uh oh!

pearu commented Apr 17, 2017

Uh oh!

juliantaylor commented Apr 17, 2017

Uh oh!

pearu commented Apr 18, 2017

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!

charris commented Apr 22, 2017

Uh oh!

charris commented Apr 25, 2017

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!

charris Apr 25, 2017 • 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!

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!

charris commented May 2, 2017

Uh oh!

charris commented May 3, 2017

Uh oh!

charris commented Nov 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

charris Apr 25, 2017 •

edited

Loading