Skip to content

BUG: Fix f2py string variables in callbacks. #10031

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

Merged
merged 2 commits into from
Nov 18, 2017

Conversation

charris
Copy link
Member

@charris charris commented Nov 15, 2017

When NPY_CHAR was deprecated and replaced by NPY_STRING in f2py, calls
to PyArray_New that previously relied on the type to get the itemsize
needed the size explicitly specified, but that modification was missed
in some of the code. Because the strings that replaced the 'c' type are
always 'S1', we use an itemsize of 1 for the string types and pass it
explicitly.

Closes #10027.

@charris charris added this to the 1.13.4 release milestone Nov 15, 2017
@charris charris requested a review from juliantaylor November 15, 2017 21:01
@charris
Copy link
Member Author

charris commented Nov 15, 2017

This still needs a test, so do not merge yet.

Test added.

@charris charris changed the title BUG: Fix string variable in callback in f2py. WIP, BUG: Fix string variable in callback in f2py. Nov 15, 2017
When NPY_CHAR was deprecated and replaced by NPY_STRING in f2py, calls
to PyArray_New that previously relied on the type to get the itemsize
needed the size explicitly specified, but that modification was missed
in some of the code. Because the strings that replacee the 'c' type are
always 'S1', we use an itemsize of 1 for the string types and pass it
explicitly.

Closes numpy#10027.
@charris charris force-pushed the fix-f2py-string-callback branch from e63f872 to 0a1069a Compare November 15, 2017 21:07
@charris charris changed the title WIP, BUG: Fix string variable in callback in f2py. WIP, BUG: Fix f2py string variables in callbacks. Nov 15, 2017
@charris charris changed the title WIP, BUG: Fix f2py string variables in callbacks. BUG: Fix f2py string variables in callbacks. Nov 15, 2017
@charris
Copy link
Member Author

charris commented Nov 15, 2017

Hmm, I think I uncovered a different error with the test. Will be fooling with stuff for a bit.

@charris charris force-pushed the fix-f2py-string-callback branch from 206d8e7 to 012ad9f Compare November 16, 2017 01:39
@charris
Copy link
Member Author

charris commented Nov 16, 2017

Should be good to go.

return 2
if not np.all(cu == b''):
return 3
return 0
Copy link
Member

@eric-wieser eric-wieser Nov 16, 2017

Choose a reason for hiding this comment

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

I'm curious - what happens if you use assert in here directly? Can f2py forward the exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know, I was tempted but figured to play safe ... Yes, that works. The fortran subroutine doesn't catch the error, rather the call wrapper does a longjmp to an error return from the fortran subroutine C wrapper.

@charris charris removed the request for review from juliantaylor November 18, 2017 01:34
@charris
Copy link
Member Author

charris commented Nov 18, 2017

Putting in for backport.

@charris charris merged commit 0688fc4 into numpy:master Nov 18, 2017
@charris charris deleted the fix-f2py-string-callback branch November 18, 2017 01:39
""",
l_not(isintent_c): """\
\tif (#name#_nofargs>capi_i) {
\t\tPyArrayObject *tmp_arr = (PyArrayObject *)PyArray_New(&PyArray_Type,#rank#,#varname_i#_Dims,#atype#,NULL,(char*)#varname_i#,0,NPY_ARRAY_FARRAY,NULL); /*XXX: Hmm, what will destroy this array??? */
\t\tint itemsize_ = #atype# == NPY_STRING ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way that VOID or UNICODE can end up here? They might need a similar fix, if so.

Copy link
Member Author

@charris charris Nov 18, 2017

Choose a reason for hiding this comment

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

I don't think so. My thought was that we will need to wait and see, the original was always defaulting. Fortran added a UCS-4 type in 2003 (name ISO_10646), but I do not see that in the current f2py. If/when we add that functionality we will need to revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Support for UCS-4 seems rather sporadic, it wasn't required until 2008. I expect utf-8 will eventually show up, but Fortran seems wedded to fixed width strings (like the NumPy S and U types) so I don't expect that to happen anytime soon.

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Nov 18, 2017
@charris charris removed this from the 1.13.4 release milestone Nov 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants