Skip to content

BUG: fix for f2py string scalars - #23194

Merged
seberg merged 6 commits into
numpy:mainfrom
2sn:f2py-string-scalar
Feb 11, 2023
Merged

seberg merged 6 commits into
numpy:mainfrom
2sn:f2py-string-scalar

Conversation

@2sn

@2sn 2sn commented Feb 10, 2023 •

Copy link
Copy Markdown
Contributor

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

seberg commented Feb 10, 2023

Copy link
Copy Markdown
Member

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

2sn commented Feb 10, 2023

Copy link
Copy Markdown
Contributor Author

OK, let me have a look.

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

2sn commented Feb 10, 2023

Copy link
Copy Markdown
Contributor Author

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

How do I execute a test?

@seberg

seberg commented Feb 10, 2023

Copy link
Copy Markdown
Member

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

2sn commented Feb 10, 2023

Copy link
Copy Markdown
Contributor Author

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

@2sn

2sn commented Feb 10, 2023

Copy link
Copy Markdown
Contributor Author

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

2sn commented Feb 10, 2023 •

Copy link
Copy Markdown
Contributor Author

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

seberg commented Feb 10, 2023

Copy link
Copy Markdown
Member

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).

Comment thread numpy/f2py/capi_maps.py
@HaoZeke

HaoZeke commented Feb 11, 2023

Copy link
Copy Markdown
Member

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

@2sn
2sn requested a review from HaoZeke February 11, 2023 11:54
@2sn

2sn commented Feb 11, 2023 •

Copy link
Copy Markdown
Contributor Author

@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=().

@HaoZeke HaoZeke left a comment

Copy link
Copy Markdown
Member

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

seberg commented Feb 11, 2023

Copy link
Copy Markdown
Member

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

2sn commented Feb 15, 2023

Copy link
Copy Markdown
Contributor Author

@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.

5 participants