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

Numpy 2 compat: replace uses of NPY_MAXARGS with a compile time constant #134

Closed
wants to merge 1 commit into from

Conversation

neutrinoceros
Copy link
Contributor

Fix #133
This is simple but fragile (it will break at runtime if numpy ever increases NPY_MAXARGS beyond the current value).
I'll open an alternative PR with a more complex and robust patch.

@neutrinoceros neutrinoceros changed the title ;80R;4R Numpy 2 compat: replace uses of NPY_MAXARGS with a compile time constant Mar 3, 2024
@neutrinoceros
Copy link
Contributor Author

Actually my alternative patch using malloc/free, is broken.

This part works fine

diff --git a/erfa/ufunc.c.templ b/erfa/ufunc.c.templ
index a904622..85b2f9a 100644
--- a/erfa/ufunc.c.templ
+++ b/erfa/ufunc.c.templ
@@ -572,7 +572,6 @@ static int ErfaUFuncTypeResolver(PyUFuncObject *ufunc,
 {
     int *types;
     PyArray_Descr **dtypes;
-    int types_array[NPY_MAXARGS];
 
     if (ufunc->userloops) {
         Py_ssize_t unused_pos = 0;
@@ -599,10 +598,12 @@ static int ErfaUFuncTypeResolver(PyUFuncObject *ufunc,
             goto fail;
         }
         /* Copy the types into an int array for matching */
+        int* types_array = malloc(NPY_MAXARGS*sizeof(int));
         for (j = 0; j < ufunc->nargs; ++j) {
             types_array[j] = ufunc->types[j];
         }
         types = types_array;
+        free(types_array);
         dtypes = NULL;
     }

But this doesn't

diff --git a/erfa/ufunc.c.templ b/erfa/ufunc.c.templ
index 1bdbd17..6ef2128 100644
--- a/erfa/ufunc.c.templ
+++ b/erfa/ufunc.c.templ
@@ -753,7 +753,8 @@ PyMODINIT_FUNC PyInit_ufunc(void)
     PyArray_Descr *dt_ymdf = NULL, *dt_hmsf = NULL, *dt_dmsf = NULL;
     PyArray_Descr *dt_sign = NULL, *dt_type = NULL;
     PyArray_Descr *dt_eraASTROM = NULL, *dt_eraLDBODY = NULL;
-    PyArray_Descr *dtypes[NPY_MAXARGS];
+    PyArray_Descr *dtypes = malloc(NPY_MAXARGS*sizeof(PyArray_Descr));
     /* ufuncs and their definitions */
     int status;
     PyUFuncObject *ufunc;
@@ -940,6 +941,7 @@ PyMODINIT_FUNC PyInit_ufunc(void)
     status = PyUFunc_RegisterLoopForDescr(
         ufunc, {{ func.user_dtype }},
         ufunc_loop_{{ func.pyname }}, dtypes, NULL);
+    free(dtypes);
     if(status != 0){
         Py_DECREF(ufunc);
         goto fail;

(compilation fails with error C2440: '=': cannot convert from 'PyArray_Descr *' to 'PyArray_Descr')
I'll admit I'm a bit out of my depth here, and generally uncomfortable with using malloc/free to patch anything, so I'm going to open the present patch for review and let reviewers decide if a more future-proofed solution is actually needed.

@neutrinoceros neutrinoceros marked this pull request as ready for review March 3, 2024 12:36
@neutrinoceros
Copy link
Contributor Author

Also: failures are thematically related in the sense that something appears to be broken in devdeps testing, but I don't think they're caused by my patch, are they ?

@mhvk
Copy link
Contributor

mhvk commented Mar 3, 2024

Hmm, the maximum number of arguments the function can get is going to be a number that depends on the erfa routines, not on numpy, so we could just determine the maximum nin+nout for any erfa function, or play it safe and just replace NPY_MAX_ARGS with 32, since that obviously was large enough.

@neutrinoceros
Copy link
Contributor Author

Oh I didn't dig enough to realise that the need didn't actually depend on numpy. I think hard-coding 32 here is sufficient. Let me push the simplest of patches !

@mhvk
Copy link
Contributor

mhvk commented Mar 3, 2024

@neutrinoceros - I think part of the problem is an incorrect numpy version constraint. I just merged #122, which gets rid of that. Can you rebase to see if that resolves the errors here?

@mhvk mhvk mentioned this pull request Mar 3, 2024
@mhvk mhvk closed this in #137 Mar 3, 2024
@neutrinoceros neutrinoceros deleted the bld/numpy_2_compat branch March 3, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly wheels are out of date ?
2 participants