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

BUG: fix for f2py string scalars #23194

Merged
merged 6 commits into from
Feb 11, 2023
Merged

BUG: fix for f2py string scalars #23194

merged 6 commits into from
Feb 11, 2023

Conversation

2sn
Copy link
Contributor

@2sn 2sn commented Feb 10, 2023

proposed fix for #23192

in previous version, any string scalar was converted to a string array of dimension len, i.e., a definition

character(len=N) :: X

effectively became

character(len=NNN), dimension(NNN) :: X

from the point of few of the numpy (python) interface:

X.shape == (NNN,)
X.dtype == '|SNNN'

@seberg seberg changed the title fix for f2py string scalars BUG: fix for f2py string scalars Feb 10, 2023
@seberg
Copy link
Member

seberg commented Feb 10, 2023

Thanks! @2sn could you please also add a very basic tests to numpy/f2py/tests/test_character.py that fails without the fix? (I think the file should be pretty straightfoward to expand hopefully.)

@2sn
Copy link
Contributor Author

2sn commented Feb 10, 2023

OK, let me have a look.

@seberg seberg added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported labels Feb 10, 2023
@2sn
Copy link
Contributor Author

2sn commented Feb 10, 2023

The tests that are present are unlike my usage, I defined in a module.

How do I execute a test?

@seberg
Copy link
Member

seberg commented Feb 10, 2023

Hmmm, I would suspect you can add it to the initial

code = ''

Inside the test class the module with that code is available as self.module. Maybe the 1, 3, star loop makes sense, in which case you include it there (I suspect that is the way to go).

I.e. insert the code here: https://github.com/numpy/numpy/blob/main/numpy/f2py/tests/test_character.py#L20 and add a test using self.module.name? Or can you not include the string definition from Fortran in the same .f90 files that gets generated there?

(Sorry, I am just trying to throw breadcrumbs that might help you, I don't have deep knowledge of f2py/Fortran to be sure this makes sense.)

@2sn
Copy link
Contributor Author

2sn commented Feb 10, 2023

I created a module test, but do I call (test) it?

@2sn
Copy link
Contributor Author

2sn commented Feb 10, 2023

well, with the old code my test of

MODULE string_test

  character(len=8) :: string

END MODULE string_test

gives

[f2py-string-scalar] ~/numpy>python -m pytest numpy/f2py/tests/test_character.py 
===================================================== test session starts ======================================================
platform linux -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
rootdir: /home/alex/numpy, configfile: pytest.ini
plugins: anyio-3.6.2, hypothesis-6.68.0
collected 44 items                                                                                                             

numpy/f2py/tests/test_character.py ...................................s.......F                                          [100%]

=========================================================== FAILURES ===========================================================
__________________________________________________ TestStringScalar.test_char __________________________________________________

self = <numpy.f2py.tests.test_character.TestStringScalar object at 0x7f6455c0f3d0>

    @pytest.mark.slow
    def test_char(self):
        out = self.module.string_test.string
        expected = ()
>       assert out.shape == expected
E       assert (8,) == ()
E         Left contains one more item: 8
E         Use -v to get more diff

expected   = ()
out        = array([b'', b' O', b'\x13', b'', b'', b'\xd5\x1a\x00\x00\x05\x00\x01\x08',
       b'\x00\x00\x00\x00&\xe9\x03', b'\x00\x1d\x00\x00\x00\x00['],
      dtype='|S8')
self       = <numpy.f2py.tests.test_character.TestStringScalar object at 0x7f6455c0f3d0>

numpy/f2py/tests/test_character.py:581: AssertionError
=================================================== short test summary info ====================================================
FAILED numpy/f2py/tests/test_character.py::TestStringScalar::test_char - assert (8,) == ()
=========================================== 1 failed, 42 passed, 1 skipped in 7.25s ============================================

but runs fine with the fix

[f2py-string-scalar*] ~/numpy>python -m pytest numpy/f2py/tests/test_character.py 
===================================================== test session starts ======================================================
platform linux -- Python 3.11.1, pytest-7.2.1, pluggy-1.0.0
rootdir: /home/alex/numpy, configfile: pytest.ini
plugins: anyio-3.6.2, hypothesis-6.68.0
collected 44 items                                                                                                             

numpy/f2py/tests/test_character.py ...................................s........                                          [100%]

================================================ 43 passed, 1 skipped in 7.34s =================================================

@2sn
Copy link
Contributor Author

2sn commented Feb 10, 2023

I am not sure the failed checks (32bit azure pipeline) are due to my code, at least I can't see any details.

RuntimeWarning: invalid value encountered in matmul

@seberg
Copy link
Member

seberg commented Feb 10, 2023

I am not sure the failed checks (32bit azure pipeline) are due to my code, at least I can't see any details.

Don't worry about it. Some heisenbug may related to openblas, maybe not, it happens occasionally (and has been for a very long time).

numpy/f2py/capi_maps.py Show resolved Hide resolved
@HaoZeke
Copy link
Member

HaoZeke commented Feb 11, 2023

Also, thanks a ton for looking into fixing this @2sn

@2sn 2sn requested a review from HaoZeke February 11, 2023 11:54
@2sn
Copy link
Contributor Author

2sn commented Feb 11, 2023

@HaoZeke The code needs to remain with rank='0' as coded. See Fortran code example above (gfortran 12.2.1-4).

As you write As per the Fortran standard, rank=0 is only for scalars, this is exactly what the fix does: setting rank=0 for scalars. dims needs to be '', consistent with the section below in the source code (later, in the calling routine this is converted to -1, which enters in the .c code generated.

if you set dims=1 and rank=1, as you suggest, then this would generate numpy array of shape=(1,) for a scalar, which is not what we want. A scalar needs to have shape=().

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@seberg
Copy link
Member

seberg commented Feb 11, 2023

Thanks a lot @2sn and @HaoZeke!

@seberg seberg merged commit 8daec0e into numpy:main Feb 11, 2023
charris pushed a commit to charris/numpy that referenced this pull request Feb 14, 2023
in previous version, any string scalar was converted to a string array of dimension len, i.e., a definition

character(len=N) :: X
effectively became

character(len=NNN), dimension(NNN) :: X
from the point of few of the numpy (python) interface:

X.shape == (NNN,)
X.dtype == '|SNNN'

Closes numpygh-23192
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 14, 2023
charris added a commit that referenced this pull request Feb 14, 2023
BUG: fix for f2py string scalars (#23194)
@2sn
Copy link
Contributor Author

2sn commented Feb 15, 2023

@seberg @HaoZeke thanks for the very quick turnaround.

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.

None yet

5 participants