BUG: Handle `iso_c_type` mappings more consistently #25226

HaoZeke · 2023-11-22T19:40:17Z

Closes #25207. This one could use a backport. Needs:

Scoping TODOs (can't rewrite the entire cfuncs here)
Tests
Check that SciPy builds

As discussed below, there were basically two deficiencies:

Type maps were not propagated
Fortran types (iso_c_binding) were mapped to ignored C types
- This is the default behavior, but is silent and the maps are done to basic (lowest precision) corresponding types

This PR fixes (pragmatically) both these deficiences, for the first problem, the fix is a simple refactor and cleanup.

The second issue is a little trickier. For now, the pragmatic solution (as done by users anyway) is to map the iso_c_binding kinds to supported f2py types, i.e. to one of:

c2py_map = {'double': 'float',
            'float': 'float',                          # forced casting
            'long_double': 'float',                    # forced casting
            'char': 'int',                             # forced casting
            'signed_char': 'int',                      # forced casting
            'unsigned_char': 'int',                    # forced casting
            'short': 'int',                            # forced casting
            'unsigned_short': 'int',                   # forced casting
            'int': 'int',                              # forced casting
            'long': 'int',
            'long_long': 'long',
            'unsigned': 'int',                         # forced casting
            'complex_float': 'complex',                # forced casting
            'complex_double': 'complex',
            'complex_long_double': 'complex',          # forced casting
            'string': 'string',
            'character': 'bytes',
            }

Pending a more complete overhaul of the generated bindings (tracked in #25229, which @Pranavchiku is also looking into) this is the optimal bugfix for #25207.

mattip · 2023-11-26T19:27:46Z

I'm not sure I can review this without some context.

HaoZeke · 2023-11-26T19:32:20Z

I'm not sure I can review this without some context.

Sorry, so the reported issue is that as currently implemented, the iso_c_binding types introduced in #24555 are not handled the same way as manually adding .f2cmap files. So this PR fixes that, instead of just merging the type dictionaries, the mapping is considered to be the same as if the user had provided it in a .f2cmap file.

I will leave comments in the code.

HaoZeke · 2023-11-26T19:32:47Z

numpy/f2py/auxfuncs.py

-def deep_merge(dict1, dict2):
-    """Recursively merge two dictionaries into a new dictionary.
-
-    Parameters:
-    - dict1: The base dictionary.
-    - dict2: The dictionary to merge into a copy of dict1.
-             If a key exists in both, the dict2 value will take precedence.
-
-    Returns:
-    - A new merged dictionary.
-    """
-    merged_dict = deepcopy(dict1)
-    for key, value in dict2.items():
-        if key in merged_dict:
-            if isinstance(merged_dict[key], dict) and isinstance(value, dict):
-                merged_dict[key] = deep_merge(merged_dict[key], value)
-            else:
-                merged_dict[key] = value
-        else:
-            merged_dict[key] = value
-    return merged_dict
-


No longer required since it is handled in process_f2cmap_dict

HaoZeke · 2023-11-26T19:33:35Z

numpy/f2py/capi_maps.py

-        for k, d1 in d.items():
-            for k1 in d1.keys():
-                d1[k1.lower()] = d1[k1]
-            d[k.lower()] = d[k]
-        for k in d.keys():
-            if k not in f2cmap_all:
-                f2cmap_all[k] = {}
-            for k1 in d[k].keys():
-                if d[k][k1] in c2py_map:
-                    if k1 in f2cmap_all[k]:
-                        outmess(
-                            "\tWarning: redefinition of {'%s':{'%s':'%s'->'%s'}}\n" % (k, k1, f2cmap_all[k][k1], d[k][k1]))
-                    f2cmap_all[k][k1] = d[k][k1]
-                    outmess('\tMapping "%s(kind=%s)" to "%s"\n' %
-                            (k, k1, d[k][k1]))
-                    f2cmap_mapped.append(d[k][k1])
-                else:
-                    errmess("\tIgnoring map {'%s':{'%s':'%s'}}: '%s' must be in %s\n" % (
-                        k, k1, d[k][k1], d[k][k1], list(c2py_map.keys())))
+        f2cmap_all, f2cmap_mapped = process_f2cmap_dict(f2cmap_all, d, c2py_map, True)


This section is used to handle user provided f2cmap files, now refactored into a function, process_f2cmap_dict which is also used for the iso_c_binding types.

Essentially, the bug in #24555 was that I didn't update f2cmap_mapped.

HaoZeke · 2023-11-26T19:34:28Z

numpy/f2py/capi_maps.py

-f2cmap_all = deep_merge(f2cmap_all, iso_c_binding_map)
+# Add ISO_C handling
+c2pycode_map.update(isoc_c2pycode_map)
+c2py_map.update(iso_c2py_map)
+f2cmap_all, _ = process_f2cmap_dict(f2cmap_all, iso_c_binding_map, c2py_map)
+# End ISO_C handling
 f2cmap_default = copy.deepcopy(f2cmap_all)


The problem with the old handling is that updating the f2cmap_all doesn't preserve / update the mapping basically.

HaoZeke · 2023-11-26T19:35:06Z

numpy/f2py/auxfuncs.py

+
+    return f2cmap_all, f2cmap_mapped


The return here is key, previously only the f2cmap dictionary was being updated, now we ensure the mapped types are tracked as well.

numpy/f2py/_isocbind.py

HaoZeke · 2023-11-26T19:46:45Z

@mattip I rescoped this into a smaller minimal fix and added comments now, the long and short of it is I forgot to consider propagating the kind map for variables my older PR (#25226 (comment)).

HaoZeke · 2023-11-26T19:56:57Z

Actually making this a draft to check the subroutine mentioned in the original issue.

Closes numpygh-25207

Fixes numpygh-25226 MAINT: Cleanup and scope fix better [f2py]

HaoZeke · 2023-11-26T20:53:52Z

numpy/f2py/_isocbind.py

-        'c_short': 'short int',
-        'c_long': 'long int',
-        'c_long_long': 'long long int',
-        'c_signed_char': 'signed char',
-        'c_size_t': 'size_t',
-        'c_int8_t': 'int8_t',
-        'c_int16_t': 'int16_t',
-        'c_int32_t': 'int32_t',
-        'c_int64_t': 'int64_t',
-        'c_int_least8_t': 'int_least8_t',
-        'c_int_least16_t': 'int_least16_t',
-        'c_int_least32_t': 'int_least32_t',
-        'c_int_least64_t': 'int_least64_t',
-        'c_int_fast8_t': 'int_fast8_t',
-        'c_int_fast16_t': 'int_fast16_t',
-        'c_int_fast32_t': 'int_fast32_t',
-        'c_int_fast64_t': 'int_fast64_t',
-        'c_intmax_t': 'intmax_t',
-        'c_intptr_t': 'intptr_t',
-        'c_ptrdiff_t': 'intptr_t',
+        'c_short': 'short',  # 'short' <=> 'int' for now
+        'c_long': 'long',  # 'long' <=> 'int' for now
+        'c_long_long': 'long_long',
+        'c_signed_char': 'signed_char',
+        'c_size_t': 'unsigned',  # size_t <=> 'unsigned' for now


What's going on here is that the original PR for ISO_C_BINDINGS was overly optimistic. F2PY can only support maps which are defined as part of the c2py keys, and then those need to have corresponding cfuncs snippets to generate binding code. For now, this performs forced casting to known types to get around that.

Eventually, the various maps in capi_maps will be updated to get more correct bindings #25229. However, for now this is exactly the handling done typically at the user end via the f2cmap files.

HaoZeke · 2023-11-26T21:25:00Z

@mattip, let me know if this is clearer now :)

Pranavchiku · 2023-11-27T16:00:50Z

I have a potential fix for #25229 ready, do you wish me to open a PR or wait to get this merged?

HaoZeke · 2023-11-27T16:47:06Z

I have a potential fix for #25229 ready, do you wish me to open a PR or wait to get this merged?

We should wait for this to get merged, but feel free to open a draft PR, we can start iterating over it.

HaoZeke · 2023-12-18T06:31:13Z

@mattip does the extended rationale make sense?

mattip · 2023-12-18T17:03:54Z

Let's try this, hoepfully we will get some end-user feedback before the release.

Thanks @HaoZeke

Fixes numpygh-25226 MAINT: Cleanup and scope fix better [f2py]

HaoZeke added 00 - Bug component: numpy.f2py labels Nov 22, 2023

HaoZeke marked this pull request as draft November 22, 2023 19:42

HaoZeke force-pushed the gh25207 branch from b96e857 to aae2e59 Compare November 22, 2023 20:51

HaoZeke marked this pull request as ready for review November 22, 2023 21:06

HaoZeke force-pushed the gh25207 branch from aae2e59 to 45137e0 Compare November 22, 2023 21:13

HaoZeke mentioned this pull request Nov 22, 2023

ENH: Add templates for f2py c functions and handle more C types #25229

Open

HaoZeke requested a review from mattip November 25, 2023 09:34

HaoZeke commented Nov 26, 2023

View reviewed changes

numpy/f2py/_isocbind.py Outdated Show resolved Hide resolved

HaoZeke marked this pull request as draft November 26, 2023 19:57

HaoZeke force-pushed the gh25207 branch from 14f3ce9 to 809bd0f Compare November 26, 2023 20:06

HaoZeke added 7 commits November 26, 2023 20:10

TST: Add one for numpygh-25207

d285c0b

BUG: Work with the ISOC_BINDINGS more consisently

e19b324

Closes numpygh-25207

MAINT: Kill now unused deep-merge [f2py]

bb46398

ENH: Populate all maps for iso_c bindings

732b155

MAINT: Correctly return mapped types [f2py]

13da8ad

DOC: Add a note on the iso_c2py_map implementation

329b1e8

MAINT: Reduce chatter to pass tests

d9b6498

HaoZeke force-pushed the gh25207 branch from 809bd0f to e7e246c Compare November 26, 2023 20:11

HaoZeke marked this pull request as ready for review November 26, 2023 20:11

HaoZeke marked this pull request as draft November 26, 2023 20:21

BUG: Map to types with bindings [f2py]

1e213c0

Fixes numpygh-25226 MAINT: Cleanup and scope fix better [f2py]

TST: Add a another test for numpygh-25207

842e39f

HaoZeke force-pushed the gh25207 branch from e7e246c to cbdcbb4 Compare November 26, 2023 20:51

HaoZeke commented Nov 26, 2023

View reviewed changes

HaoZeke marked this pull request as ready for review November 26, 2023 20:55

HaoZeke force-pushed the gh25207 branch from cbdcbb4 to 842e39f Compare November 26, 2023 21:00

Pranavchiku mentioned this pull request Nov 27, 2023

ENH: Add templates for f2py c functions and handle more C types #25260

Draft

HaoZeke requested a review from melissawm November 28, 2023 19:33

charris added the 09 - Backport-Candidate PRs tagged should be backported label Dec 18, 2023

mattip merged commit 003c846 into numpy:main Dec 18, 2023

HaoZeke deleted the gh25207 branch December 19, 2023 00:25

charris pushed a commit to charris/numpy that referenced this pull request Dec 23, 2023

BUG: Map to types with bindings [f2py]

599e427

Fixes numpygh-25226 MAINT: Cleanup and scope fix better [f2py]

charris changed the title ~~BUG: Handle iso_c_type mappings more consistently~~ BUG: Handle iso_c_type mappings more consistently Dec 23, 2023

charris mentioned this pull request Dec 23, 2023

BUG: Handle iso_c_type mappings more consistently #25480

Merged

3 tasks

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 23, 2023

HaoZeke restored the gh25207 branch June 15, 2024 23:40

HaoZeke deleted the gh25207 branch January 18, 2025 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Handle `iso_c_type` mappings more consistently #25226

BUG: Handle `iso_c_type` mappings more consistently #25226

HaoZeke commented Nov 22, 2023 •

edited

Loading

mattip commented Nov 26, 2023

HaoZeke commented Nov 26, 2023

HaoZeke Nov 26, 2023

HaoZeke Nov 26, 2023

HaoZeke Nov 26, 2023

HaoZeke Nov 26, 2023

HaoZeke Nov 26, 2023

HaoZeke commented Nov 26, 2023

HaoZeke commented Nov 26, 2023

HaoZeke Nov 26, 2023 •

edited

Loading

HaoZeke commented Nov 26, 2023

Pranavchiku commented Nov 27, 2023

HaoZeke commented Nov 27, 2023

HaoZeke commented Dec 18, 2023

mattip commented Dec 18, 2023

BUG: Handle iso_c_type mappings more consistently #25226

BUG: Handle iso_c_type mappings more consistently #25226

Conversation

HaoZeke commented Nov 22, 2023 • edited Loading

mattip commented Nov 26, 2023

HaoZeke commented Nov 26, 2023

HaoZeke Nov 26, 2023

Choose a reason for hiding this comment

HaoZeke Nov 26, 2023

Choose a reason for hiding this comment

HaoZeke Nov 26, 2023

Choose a reason for hiding this comment

HaoZeke Nov 26, 2023

Choose a reason for hiding this comment

HaoZeke Nov 26, 2023

Choose a reason for hiding this comment

HaoZeke commented Nov 26, 2023

HaoZeke commented Nov 26, 2023

HaoZeke Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

HaoZeke commented Nov 26, 2023

Pranavchiku commented Nov 27, 2023

HaoZeke commented Nov 27, 2023

HaoZeke commented Dec 18, 2023

mattip commented Dec 18, 2023

BUG: Handle `iso_c_type` mappings more consistently #25226

BUG: Handle `iso_c_type` mappings more consistently #25226

HaoZeke commented Nov 22, 2023 •

edited

Loading

HaoZeke Nov 26, 2023 •

edited

Loading