Repository navigation
BUG: incompatibility between 1.23 and 1.24 for f2py generated interface to string arrays #23356
Description
Describe the issue:
Since NumPy version 1.24, f2py generates for Fortran arrays of strings (e.g. CHARACTER WORD(3) * 8) a NumPy array object with dtype |S1, where the string length is added as a last dimension of the array (so the shape for the example in 1.24 is (3, 8)). In prior versions the array would have a dtype with the size of the Fortran string and the same dimensions as the Fortran type.
Reproduce the code example:
Fortran code (test.f):
PROGRAM TEST
CHARACTER WORD*8
CHARACTER WORDARR(3)*8
CHARACTER WORDARR2(3, 4)*8
COMMON /BLOCK/ WORD, WORDARR, WORDARR2
END
Command to generate Python module:
python -m numpy.f2py -c test.f -m test
Python test script:
import test
for attr in ('word', 'wordarr', 'wordarr2'):
var = getattr(test.block, attr)
print(attr, var.shape, var.dtype)Error message:
Version 1.23.5 output:
word () |S8
wordarr (3,) |S8
wordarr2 (3, 4) |S8
Version 1.24.2 output:
word (8,) |S8
wordarr (3, 8) |S1
wordarr2 (3, 4, 8) |S1Runtime information:
>>> import sys, numpy; print(numpy.__version__); print(sys.version); print(numpy.show_runtime())
1.24.2
3.10.4 (main, Mar 31 2022, 08:41:55) [GCC 7.5.0]
[{'simd_extensions': {'baseline': ['SSE', 'SSE2', 'SSE3'],
'found': ['SSSE3',
'SSE41',
'POPCNT',
'SSE42',
'AVX',
'F16C',
'FMA3',
'AVX2'],
'not_found': ['AVX512F',
'AVX512CD',
'AVX512_KNL',
'AVX512_KNM',
'AVX512_SKX',
'AVX512_CLX',
'AVX512_CNL',
'AVX512_ICL']}},
{'architecture': 'Haswell',
'filepath': '/jop91_0/eldering/anaconda3/envs/test/lib/python3.10/site-packages/numpy.libs/libopenblas64_p-r0-15028c96.3.21.so',
'internal_api': 'openblas',
'num_threads': 8,
'prefix': 'libopenblas',
'threading_layer': 'pthreads',
'user_api': 'blas',
'version': '0.3.21'}]
None
Context for the issue:
The issue of WORD having both a shape of (8,) and a dtype of |S8 is solved by #23194. But the differences between version 1.23 and 1.24 for WORDARR and WORDARR2 remain.
The issue of equivalence between string length and array dimensions is mentioned here #19388. But I didn't get from that discussion that the way version 1.24 treats string arrays is an intended feature. Also, this difference isn't mentioned in the release notes. So I assume it is a bug. Please correct me if I'm mistaken.
Activity
charris commentedon Mar 7, 2023
@HaoZeke Thoughts?
bobeldering commentedon Mar 13, 2023
I tried to understand what changed with version 1.24. The most relevant change to this problem seems to be the removal of the special case handling of strings around line 145 of fortranobject.c.
This special case previously used the last dimension of the array as the element size and then decreased the rank.
Another bit of code that might be relevant (and gets me quite confused) is in crackfortran.py
Here the reverse of the above special case is executed: adding the element size to the dimensions of an array.
seberg commentedon Mar 16, 2023
Maybe @2sn you have a quick idea, just wondering if the fix may be to extend the older PR with an array path?
2sn commentedon Mar 16, 2023
@seberg I am not quite sure I understand the example (I never wrote fortran code that uses the type declarations for strings in that way). I can confirm that in some old version of f2py it did not like the
len=parameter, and sometimes it used to produce an array of char 1 of length of the string. I had written some wrappers for that to get the strings in python I needed. The new version seemed to fix that except the was an actual bug in f2py that it it took the sting length and also generated an array length of that value of that length of string - obviously beyond memory boundaries. The PR I contributed just fixes that for scalar strings and may also address this oversight in the 1.24.2 NumPy version. I think it may also fix some of the cases shown above.That now should definitely return
This was my PR.
I thought these two cases
would (certainly should) still do the same as listed above for 1.23.5. But I do not recall whether I tested it.
When do we expect a release of NumPy 1.24.3?
seberg commentedon Mar 16, 2023
Hmmm, not sure we have a concrete plan for 1.24.3 yet, so if we have some follow-ups to fix this, we could probably include it. Or are you just hoping for 1.24.3 soon?
There may be a few issues to mop-up, but I suspect we could do a release pretty soon.
2sn commentedon Mar 16, 2023
I just tried out the current main branch. Using the definitions above within a module for a current project (for the sake of a quick test) I get
So, my PR is included and fixes the scalar case, as it should, but I have included a test that checks the array cases work as well. So I was confused what happened here, whether there have been other changes to f2py since, and whether the test was ignored or was changed as well. Or I whether messed it up. The last two cases should not have had the last dimension by should have had dtype
|S8- at least that is what I would find desirable.You can see these tests in
numpy/f2py/tests/src/string/scalar_string.f90and
numpy/f2py/tests/test_character.pyof my PR, testing exactly that. So I was not sure what happened to these test in the current main branch. The tests seem there. How can it not work?
So I gave it another try, and change definition to modern fortran:
Now this gives the expected results, that are also in the test case of my PR:
The issue must then be that current crackfortran does not properly interpret the construct
and takes the second number as a dimension as well, whereas it interprets modern format specification correctly.
So, unless someone knows crackfortran well enough to debug this dated fortran code case, maybe it is an option for @bobeldering to modify the type specification in the quoted Fortran code to a more modern format that is implemented in f2py correctly?
bobeldering commentedon Mar 22, 2023
With the knowledge that the parsing works for Fortran 90 style string arrays, I had another look at the Fortran parsing
As I understand it, the pair of functions
cracktypespec0andcracktypespecparse the type definition as far as it is in front of the variable name. The part after the variable name is parsed inupdatevars.In
updatevars, the dict from the regular expression match parsing the type definition after the variable name, is updated for a few cases. One of those cases adds the length to the dimensions and removes the length (if the parsed type has both a length and dimensions). Another transforms the length to acharselectororkindselector, but since this case is handled after the previous one, it isn't evaluated for string arrays.I made a patch, which fixes the problem for this test case, by switching the order in which these checks are handled. The patch passes all the f2py test. But as I mentioned before, I'm quite confused about the purpose of the case where the length is added to the dimensions. It clearly used to produce the correct results with the special case handling of strings in fortranobject.c (that was removed in 1.24). I can make a PR for this patch, but I'm curious if anybody knows what the purpose of these lines of code is (was?).
seberg commentedon Mar 24, 2023
@bobeldering do you want to make a PR for the patch? It might be easier to get someone to comment on that. Is the
if 'len' in d1 and 'array' in d1:line maybe relevant for numeric arrays running into the same thing?BUG: Fix bug in parsing F77 style string arrays.
bobeldering commentedon Mar 27, 2023
I extended the test case with some types from the first Fortran77 documentation I could find, to check how it handles number types:
Printing the shape and dtype gave some interesting results:
So both the dtype and shape for the
* 8types seems off to me. However, versions 1.23.5, 1.24.2 and the patched version all produce the same results for the non-character based types. So I consider this a possibly related, but separate bug and made a PR.2sn commentedon Mar 27, 2023
@bobeldering I would say the character types look OK with dtype
|S, however, thelnumarseems broken, all long and double types a are just the single and real counterparts, which seems very wrong. Even long floats end up as int.Could you try what happens if you use
implicit none(as all your variables that starlandiand up as single ints)?Are there any type-overwrites enabled in the fortran compiler flags?
bobeldering commentedon Mar 27, 2023
@2sn I think you are correct that the types are implicitly derived from the variable name. Changing the L to a Z gives:
Adding
implicit nonegives a strong hint that the parsing failed, f2py generates extra warning messages:So it looks like the
* 8s are not parsed in the pre variable name type definition, but instead prepended to the variable name. And possibly float32 is just the default type?2sn commentedon Mar 27, 2023
@bobeldering I still recommend to try putting in an
implicit nonestatement. Apparently long and double type spects are still ignored. Which is really strange. This should have worked traditionally. Hence I also wonder whether there are any type overwrites in the compile flags. Something line -i8-i4 or so.bobeldering commentedon Mar 28, 2023
The warnings messages occur with the
implicit nonestatement added. I forgot to mention that this attempt eventually failed to build. The complete output is:NumPy 1.24 generates a wrappers for the Fortran code that are not con…
2sn commentedon Mar 28, 2023
@bobeldering Yes, I can reproduce your findings. W/o the
implicit noneI get fromthe output
but when I add it I also get the error message
gfortranitself, however, compiles it just fine, so the issue must lay withupdatevars. Apparently, it does not know how to handle this pattern. May I ask, is there any specific reason not to update to modern f90-style declarations? If you have lots of code, I presume an AI could handle that for you.2sn commentedon Mar 28, 2023
result
This compiles and gives the result
as desired.
seberg commentedon Mar 28, 2023
(Didn't quite follow the discussion) As long as we are confident that the suggest fix doesn't break the numeric things which used to work, we could just apply and backport it, though? Even if the better fix is to use f95-style declarations in the mid-term, probably.
That is, if you two are pretty confident about that being the case?
2sn commentedon Mar 28, 2023
I did not review the fix. So I can't advise. My apologies.
2sn commentedon Mar 28, 2023
@seberg I think that most likely a small addition or adjustment of a pattern in
updatevarsmight fix it, but I do not have time at the present to look into it. But in general, I think it would indeed be preferable for f2py being able to compile f77-style codes as well. Although I no longer use f77-style code (all re-written during lock-downs).bobeldering commentedon Mar 28, 2023
The Fortran 77 code I'm using f2py for is large-ish; about 80k lines. The general idea is to slowly change the code base to Python (when changes to the functionality are required) and use f2py to generate the wrappers for the places where the Python code still calls the old Fortran code. I prefer to touch the Fortran code as little as possible, reasons for that include that I'm not very familiar with Fortran and it would complicate incorporating patches to the Fortran code (I don't expect many, but it might still happen).
The numeric types I added to the example above were just for testing whether the patch affects them. The Fortran code I'm working with doesn't use those numeric types, neither does is use implicit typing. The only problem I'm running into with NumPy 1.24 is the handling of string arrays declared like
CHARACTER WORDARR(3)*8.2sn commentedon Mar 28, 2023
@bobeldering My advice would be to
BUG: Fix bug in parsing F77 style string arrays.
seberg commentedon Apr 25, 2023
This should have been fixed by gh-23637 (and the original PR).
NumPy 1.24 generates a wrappers for the Fortran code that are not con…