Conversation
danieldouglas92
left a comment
danieldouglas92
left a comment
There was a problem hiding this comment.
This looks great to me!
|
I'm not sure why the periodic_box_mpi4 test would be failing with these changes |
gassmoeller
left a comment
gassmoeller
left a comment
There was a problem hiding this comment.
I like the direction to collect python parts in python_helper.h. I have a few questions, maybe I am just not understanding the purpose correctly.
include/aspect/python_helper.h
Outdated
|
|
||
| #define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION | ||
|
|
||
| // |
There was a problem hiding this comment.
This needs a comment. What is the purpose of PY_ARRAY_UNIQUE_SYMBOL, and what is ASPECT_ARRAY_API?
include/aspect/python_helper.h
Outdated
| // ASPECT_NUMPY_DEFINE_API in exactly one translation unit per | ||
| // executable/shared library (in main.cc and any plugin that uses | ||
| // Python). All other translation units including this header will | ||
| // therefor define NO_IMPORT_ARRAY that asks numpy to skip defining |
There was a problem hiding this comment.
| // therefor define NO_IMPORT_ARRAY that asks numpy to skip defining | |
| // therefore define NO_IMPORT_ARRAY that asks numpy to skip defining |
| #endif | ||
|
|
||
| #define ASPECT_NUMPY_DEFINE_API | ||
| #include <aspect/python_helper.h> |
There was a problem hiding this comment.
does ASPECT_NUMPY_DEFINE_API need to be undefined after the include (for unity builds where other translation units may be combined with main.cc)?
There was a problem hiding this comment.
Good point, thank you! I am now #undefing both
There was a problem hiding this comment.
Do you still need the #define here, though?
There was a problem hiding this comment.
Yes! in exactly one translation unit I need to define this macro (see description in the header file)
| return PyArray_SimpleNewFromData(1, &size, NPY_DOUBLE, data); | ||
| } | ||
| #define ASPECT_NUMPY_DEFINE_API | ||
| #include <aspect/python_helper.h> |
There was a problem hiding this comment.
same question here, shouldnt we undefine ASPECT_NUMPY_DEFINE_API after the include?
|
Hm, yes and I dont know why the test fails either. Maybe it is unrelated to this PR, we could have picked up that test failure in the past weeks while the deal.II development tester didnt work for us. |
|
I checked and |
Is this a fallout from the "ghosted vector" changes? |
|
I think I addressed all your points. |
| #ifdef ASPECT_NUMPY_DEFINE_API | ||
| #undef ASPECT_NUMPY_DEFINE_API | ||
| #endif | ||
| #ifdef NO_IMPORT_ARRAY | ||
| #undef NO_IMPORT_ARRAY | ||
| #endif |
There was a problem hiding this comment.
| #ifdef ASPECT_NUMPY_DEFINE_API | |
| #undef ASPECT_NUMPY_DEFINE_API | |
| #endif | |
| #ifdef NO_IMPORT_ARRAY | |
| #undef NO_IMPORT_ARRAY | |
| #endif | |
| #ifdef ASPECT_NUMPY_DEFINE_API | |
| # undef ASPECT_NUMPY_DEFINE_API | |
| #endif | |
| #ifdef NO_IMPORT_ARRAY | |
| # undef NO_IMPORT_ARRAY | |
| #endif |
include/aspect/python_helper.h
Outdated
| /** | ||
| * Convert a std::vector<double> to a numpy array. | ||
| * @param[in] vec The vector to convert. | ||
| * @return A PyObject* to the numpy array. | ||
| */ | ||
| inline PyObject *vector_to_numpy_object(const std::vector<double> &vec) | ||
| { | ||
| npy_intp size = static_cast<npy_intp>(vec.size()); | ||
| if (size == 0) | ||
| return PyArray_SimpleNew(1, &size, NPY_DOUBLE); | ||
| double *data = const_cast<double *>(vec.data()); | ||
| return PyArray_SimpleNewFromData(1, &size, NPY_DOUBLE, data); |
There was a problem hiding this comment.
Could you comment on whether the object has to be explicitly destroyed? I imagine so, and perhaps in that case it would be useful to return not just a plain pointer, but a std::unique_ptr<PyObject, void(*)(PyObject*)> where when you create the object, you add a lambda that calls the destructor on the PyObject.
| #endif | ||
|
|
||
| #define ASPECT_NUMPY_DEFINE_API | ||
| #include <aspect/python_helper.h> |
There was a problem hiding this comment.
Do you still need the #define here, though?
|
I am not a big fan of the unique_ptr approach, but it works. |
4 participants
Python and the numpy C API make it very difficult to use in several translation units due to the
import_array()facility that internally defines some static functions/members. It took me some time to get this right, but it now works correctly for me. I decided to move the shared functionality into a new headerpython_helper.h.@bangerth will hopefully like the idea that we can put helper functions there and use them from several places.
FYI @danieldouglas92