MAINT, BUG: Remove uses of PyString_FromString. by charris · Pull Request #17068 · numpy/numpy

charris · 2020-08-11T21:40:46Z

We no longer need to use the compatibility function after dropping support for
Python 2.7. In some cases unicode was the correct string type rather than the
bytes of the compatibility version and bugs in the array __complex__ and
array __array_interface__ methods have been fixed by changing that.

eric-wieser · 2020-08-11T22:53:24Z

Apologies @charris, I thought I was reviewing code from @jakobjakobson13! Identicons are apparently too similar at the end of the day.

eric-wieser · 2020-08-11T23:50:55Z

Hmm, if this is auto-generated then maybe we should not be editing it?

It's in the repo, doesn't go away with git clean -xdfq and the content dates back to 2005. I think the string is intended for code generated by wrapper.c. ~~However, in that case it is probably best to keep the message in bytes...~~ It is a docstring for modules using wrapper.c and is directly set during module init. The tests pass with it as patched.

I think the string is intended for code generated by

I think you're right, but the top of this file also says its autogenerated.

Probably just left over from before the tests were redone. Note that it is only used in setting up test_array_from_pyobj.py, so I think it really doesn't matter.

The wrapmodule.c is used in numpy/f2py/tests/test_array_from_pyobj.py and can be considered constant, that is, it can be manually modified.
For background: he file was generated with f2py as a C/API module template and later modified manually to expose various macro constants to Python for testing..

Sounds like that part of the header can just be removed. I'll leave this PR as is with a release note and make a separate PR with the version changes and other small cleanups suggested here.

Would you like to wait for this to be merged until the follow-on PR is made?

I don't understand your question @mattip, the header is changed already.

@mattip No :) I'm waiting for this to be merged. The follow-on PR just changes the version handling.

eric-wieser · 2020-08-12T08:38:23Z

It's tempting to split this into two phases:

Perform dumb text substitution of all the compat macros in one PR
Comb through the uses of PyBytes_... to check for things which don't make sense, and make as many PRs as needed. I can't think of many situations where bytes are the correct choice in numpy.

charris · 2020-08-12T15:48:40Z

It's tempting to split this into two phases:

I agree that we need to replace all the compat functions, but would rather do it in smaller pieces. Look how much work this small bit is already taking :) I think blind replacement just makes it harder to find the uses that may need fixing.

seberg · 2020-08-12T15:54:15Z

Right now, I am still happy to keep it here, since its not time pressing to do the replacement and the PR is not big.

The remaining issue is mainly the f2py change, right? It sounds like most of the other actual changes were fairly clear to resolve?

I suppose for some of the things that end up being fixes, including a test might be nice (also helps with judging whether anything should have release notes), but not sure.

pearu · 2020-08-18T20:12:26Z

I just noticed this PR and I'd like to review the f2py bits. However, it's late here but I can review tomorrow.

charris · 2020-08-18T20:16:45Z

@pearu Great. Other functions proposed for replacement in #17097 also appear in the f2py code so this may not be the last time review is needed.

pearu

LGTM from the f2py point of view.

While slightly OT, added some suggestions for updating f2py version handling.

pearu · 2020-08-19T13:21:06Z

The wrapmodule.c is used in numpy/f2py/tests/test_array_from_pyobj.py and can be considered constant, that is, it can be manually modified.
For background: he file was generated with f2py as a C/API module template and later modified manually to expose various macro constants to Python for testing..

eric-wieser · 2020-08-19T14:32:18Z

Think it's worth adding a test for the code snippets I give above that expose what this fixed?

At any rate, this should be BUG: or BUG,MAINT or however we combine things.

We no longer need to use the compatibility function after dropping support for Python 2.7. In some cases unicode was the correct string type rather than the bytes of the compatibility version and bugs in the array `__complex__` and array `__array_interface__` methods have been fixed by changing that.

charris · 2020-08-19T15:37:57Z

Think it's worth adding a test for the code snippets I give above

Been thinking about that. We could add those to the regressions checks, but in the long run it would be useful to figure out how to generate a set of comprehensive tests for both errors and descriptors. @seberg may have some ideas about the latter.

- Invalid conversion error bug. - ``__array_interface__['descr']`` bug

eric-wieser

Thanks for the tests. I think with @pearu's review of the f2py stuff, this is now good to go.

mattip · 2020-08-20T12:55:36Z

I was asking generally about this PR: there are follow-on cleanups:

in numpy/numpy/core/src/multiarray/ctors.c where we call _array_typedescr_fromstr after converting a str to bytes then back to str
changes in line 205 to numpy/f2py/rules.py to use f2py_versionand delete line 53

I guess we should merge this now and then submit a follow-on PR. In it goes.

mattip · 2020-08-20T12:56:06Z

Thanks @charris

charris added component: numpy.linalg component: numpy._core component: numpy.f2py 03 - Maintenance labels Aug 11, 2020

jakobjakobson13 mentioned this pull request Aug 11, 2020

List of possible bugs found by automated code analysis #17012

Open

28 of 35 tasks

charris force-pushed the cleanup-pystring_fromstring-usage branch from 4ed2384 to 9915e28 Compare August 11, 2020 21:54