-
Notifications
You must be signed in to change notification settings - Fork 0
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
MAINT: Check numpy version in C-API #1
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
#ifndef NUMPY_CORE_INCLUDE_NUMPY_UTILS_H_ | ||
#define NUMPY_CORE_INCLUDE_NUMPY_UTILS_H_ | ||
|
||
#include <Python.h> | ||
|
||
#ifndef __COMP_NPY_UNUSED | ||
#if defined(__GNUC__) | ||
#define __COMP_NPY_UNUSED __attribute__ ((__unused__)) | ||
|
@@ -34,4 +36,17 @@ | |
#define NPY_CAT_(a, b) NPY_CAT__(a, b) | ||
#define NPY_CAT(a, b) NPY_CAT_(a, b) | ||
|
||
static PyObject *_npy_import_numpy_multiarray_umath() | ||
{ | ||
PyObject *multiarray = PyImport_ImportModule("numpy._core._multiarray_umath"); | ||
if ( | ||
multiarray == NULL && | ||
PyErr_ExceptionMatches(PyExc_ModuleNotFoundError) | ||
) { | ||
PyErr_Clear(); | ||
multiarray = PyImport_ImportModule("numpy.core._multiarray_umath"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is a try/except and somewhat legal, but not as is, you need to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! I updated it. |
||
} | ||
return multiarray; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using this to get the major version is a bit overloaded IMO, since we also have it available as a C function in the API table. That said, we can probably probably add such a helper, although I think it should also be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I am not sure it was clear: I would suggest if we have a helper, it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @seberg How about In autogenerated headers (A downstream lib could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, I am not seeing/understanding this. Those should be private headers, and even if the function exposed there isn't used in I would be surprised to find any use of this in public API accidentally or not. But, it wouldn't be the first surprise like this :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I finished refactoring - I ended up with a simpler version. |
||
|
||
#endif /* NUMPY_CORE_INCLUDE_NUMPY_UTILS_H_ */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this, it should already say something similar? But OK.
Generally seems fine, I am not immediately sure if
utils.h
is a good place for the helper.The one worry I would have is that we might get a
ModuleNotFoundError
also because_multiarray_umath
is missing (broken install). However, not sure I want to worry too much about it, since I suspect we will have enough info available andnumpy
is probably more often imported in Python first anyway.