Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate NPY_CHAR #2801

Closed
rgommers opened this issue Dec 9, 2012 · 25 comments
Closed

Deprecate NPY_CHAR #2801

rgommers opened this issue Dec 9, 2012 · 25 comments
Labels
07 - Deprecation 17 - Task Priority: high High priority, also add milestones for urgent issues

Comments

@rgommers
Copy link
Member

rgommers commented Dec 9, 2012

This broke ABI compatibility in 1.6.x. Should be deprecated and removed, see http://projects.scipy.org/numpy/ticket/2228 (dead link)

Archive of a sort here

@certik
Copy link
Contributor

certik commented Dec 14, 2012

@rgommers, how do I deprecate it? In the release notes? Should this be then removed from master but kept in the 1.7 branch? How exactly should this be removed from the master branch? Just removing it from the NPY_TYPES enum?

Thanks for any tips here.

P.S. Where is the github issue corresponding to the ticket 2228? I was not able to find it. I thought all issues were migrated to github...

@rgommers
Copy link
Member Author

The Trac tickets with numbers higher than 2225 or so don't have Github equivalents. Trac still has to be made read-only, then the last tickets should be migrated.

Deprecation should be noted in the release notes. It would also be good to use NPY_NO_DEPRECATED_API if possible to generate compiler warnings. See http://docs.scipy.org/doc/numpy-dev/reference/c-api.deprecations.html. Disclaimer: I haven't checked if and how that would work with NPY_CHAR exactly.

@njsmith
Copy link
Member

njsmith commented Dec 15, 2012

It's extremely difficult to get a compiler warning out of use of an enum. I guess we could exploit the C compiler extensions for deprecations in some complicated way -- maybe give the enum a different name like NPY_CHAR_REAL and then

static const int NPY_CHAR = NPY_CHAR_REAL UGLY_COMPILER_SPECIFIC_DEPRECATION_MACRO;

?

If we've already broken this multiple times and the only code that anyone can find that uses it is unmaintained, then maybe we should just remove it... no-one who actually has NPY_CHAR-using code is going to see the warnings anyway, or bother setting NPY_NO_DEPRECATED_API... that's an opt-in thing for diligent maintainers.

@rgommers
Copy link
Member Author

@njsmith I agree that it's not worth spending a lot of effort on. Deprecating in the release notes for 1.7 and removing it for 1.8 should be OK too.

@certik
Copy link
Contributor

certik commented Dec 15, 2012

@rgommers, can you please review the PR I just sent?

@njsmith
Copy link
Member

njsmith commented Dec 15, 2012

So after that PR gets merged, the other thing to do is to get a PR to remove NPY_CHAR entirely from master?

@rgommers
Copy link
Member Author

Indeed. Let's give that some time though, usually it's not a good idea to rip something out right after deprecating it. I'll change the Milestone for this issue to 1.8.

@certik
Copy link
Contributor

certik commented Dec 15, 2012

Thanks!

nouiz pushed a commit to nouiz/numpy that referenced this issue Dec 17, 2012
@njsmith
Copy link
Member

njsmith commented Mar 6, 2013

Time to rip this out of master!

@charris
Copy link
Member

charris commented Mar 6, 2013

+1

@njsmith
Copy link
Member

njsmith commented Apr 11, 2013

I looked at this but swiftly got confused. (Perhaps because I have no idea what NPY_CHAR actually is.) Specifically:

  • What's the relation between NPY_CHAR and NPY_CHARLTR? I suppose both should be removed?
  • What do we want to do about f2py? From numpy/f2py/capi_maps.py:
    c2capi_map={  # ...
            'string':'NPY_CHAR', # f2py 2e is not ready for NPY_STRING (must set itemisize etc)
            #'string':'NPY_STRING'

I suppose we could just remove it and see who screams, but someone here might have a better idea of what would happen if we tried...

@rgommers
Copy link
Member Author

@pearu could you comment on the f2py question above?

@pearu
Copy link
Contributor

pearu commented Apr 15, 2013

Current f2py users certainly use NPY_CHAR as character array arguments are quite common in Fortran world.
Removing NPY_CHAR without updating f2py to use NPY_STRING will break the code of those users when upgrading numpy.

To wrap Fortran character arrays, f2py uses NPY_CHAR because at the time of developing f2py, numpy did not have NPY_STRING. Now that NPY_STRING exists, f2py should use it. In fact, it would make wrapping Fortran character arrays to Python much more natural. Currently, f2py users must play a little bit to get hang of how to manage character array arguments.

Implementing NPY_STRING support has been in my TODO list for years. Unfortunately due to the lack of time as well as more cumbersome development cycle of numpy [1], I had to postpone this task to future again and again. I'll try to locate some time and update f2py to use NPY_STRING.

[1] "It's me, not you". In past it was much easier to fix small bugs or develop some feature in numpy.f2py than nowadays, as committing patches was straightforward with subversion. I use git rarely and each time I spend more time to commit patches (due to my git ignorance) than developing a particular patch. I have two options: learn git or create a svn repo of latest numpy and develop f2py from there. The latter would be quickest for me to get things done.

@rgommers
Copy link
Member Author

@pearu thanks for the explanation. If it helps you get this done, I'm happy to take your svn repo commits or emailed patches and apply them.

@certik
Copy link
Contributor

certik commented Apr 15, 2013

Hi @pearu! I think it's a great investment to learn git. But if you prefer subversion, you can use subversion with github easily: https://github.com/blog/1178-collaborating-on-github-with-subversion

@pearu
Copy link
Contributor

pearu commented Apr 21, 2013

Thanks, Certik, for the hint. However, the suggested workflow does not work with numpy. For instance, I cannot commit a newly created branch (it appears to be forbidden). I'll look other options..

@pearu
Copy link
Contributor

pearu commented Apr 21, 2013

Ok, I was able to get svn working with github using my numpy fork. I'll let you know when its ready for pull.

@njsmith
Copy link
Member

njsmith commented Apr 25, 2013

Guess we'll probably have to bump this to 1.9.

@charris
Copy link
Member

charris commented Feb 21, 2014

@pearu Any progress?

@charris charris modified the milestones: 1.10 blockers, 1.9 blockers May 5, 2014
@charris
Copy link
Member

charris commented May 5, 2014

Moving up to 1.10 blockers.

@charris
Copy link
Member

charris commented Jun 21, 2015

Moving up to 1.11.

@charris charris modified the milestones: 1.11 blockers, 1.10 blockers Jun 21, 2015
@charris charris modified the milestones: 1.12.0 release, 1.11.0 blockers Jan 21, 2016
@auvipy
Copy link

auvipy commented Mar 26, 2016

is this deprecated yet? if not how to deprecate a numpy feature? i'm aware with django internals only

@rgommers rgommers modified the milestone: 1.12.0 release Feb 15, 2017
@juliantaylor
Copy link
Contributor

@pearu any progress?
Can you atleast outline what needs to be done?
this is really important is it is blocking the addition of any new core dtypes.

@juliantaylor juliantaylor added this to the 1.13.0 release milestone Apr 14, 2017
@juliantaylor
Copy link
Contributor

gh-8948 for the deprecation.

@charris
Copy link
Member

charris commented May 5, 2017

I believe this has been closed by #8948.

@charris charris closed this as completed May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
07 - Deprecation 17 - Task Priority: high High priority, also add milestones for urgent issues
Projects
None yet
Development

No branches or pull requests

7 participants