-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix problems with Python, improve installation, and improve tests #466
Conversation
Thanks for the review, @JulienDoerner I tried to make the installation default to I've made one further change: I commented out the test that was leading to the problem in the OSX tests. Unless you have any objections, I'd suggest we keep it that way until we figure out how to solve the problem. And it is better not to delete the corresponding lines yet, since they might be useful in the future. |
It's more or less working for me. However, I would also prefer an option to specify the CMAKE_INSTALL_PREFIX and Python_INSTALL_PACKAGE_DIR independently of each other. I need to set the CMAKE_INSTALL_PREFIX to something, which is different from the default /usr/local as we do not have write access to that location on our university machines. This leads in the current PR to installing the python module in CMAKE_INSTALL_PREFIX/crpropa which would require manually adapting the python PATH for my virtual envs... EDIT: |
Hey @rafaelab, I think adding this variable And also this does not reproduce the old installation behaviour. In this case (given PREFIX) all installation parts are stored on the same level (python package |
Thanks for the feedback, @JulienDoerner and @lukasmerten. The remaining issue is what @JulienDoerner said about |
hey @rafaelab, I think the change is good, as it is. My point for the |
Thanks for your effort @rafaelab. Just for my records: Just for curiosity: |
No idea, @lukasmerten. I did add a status message with the value of this flag, but I haven't invested too much time in adding it to the parameter. I thought it would show up there automatically. |
PyArrayObject *arr = NULL; | ||
PyArray_Descr *dtype = NULL; | ||
int ndim = 0; | ||
npy_intp dims[NPY_MAXDIMS]; | ||
if (PyArray_GetArrayParamsFromObject(input, NULL, 1, &dtype, &ndim, dims, &arr, NULL) < 0) | ||
{ | ||
if (PyArray_GetArrayParamsFromObject(input, NULL, 1, &dtype, &ndim, dims, &arr, NULL) < 0) { |
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.
Something goes wrong on this line. See issue #448 . Please, check behavior of this code on NumPy version 1.21.6.
This PR supersedes #457.
It mainly improves the handling of python, but it also contains many other improvements/fixes:
EXPECT_THROW
)testInteraction
related to EMPairProduction, in OS X.One of the OSX tests (
testPythonExtension.py
) is expected to fail because of a problem applying NumPy functions toVector3d
in one of the tests (testArrayInterface
). This does not happen, for example, in Ubuntu.Even if this particular test fails, at least it is known and doesn't seem to affect CRPropa in any significant way.