CRPropa / CRPropa3

CRPropa is a public astrophysical simulation framework for propagating extraterrestrial ultra-high energy particles. https://crpropa.github.io/CRPropa3/
https://crpropa.desy.de
GNU General Public License v3.0
65 stars 66 forks source link

Fix problems with Python, improve installation, and improve tests #466

Closed rafaelab closed 4 months ago

rafaelab commented 5 months ago

This PR supersedes #457. It mainly improves the handling of python, but it also contains many other improvements/fixes:

One of the OSX tests (testPythonExtension.py) is expected to fail because of a problem applying NumPy functions to Vector3d 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.

rafaelab commented 5 months ago

I'm puzzled by the fact that the crpropa-example-test is passing in my branch and not here. That is very weird.

JulienDoerner commented 5 months ago

@rafaelab: The failing test is related to the notebook secondaries\photons.ipynb, but it got two different erros in the two executions. I will try to run it a thrid time. We had the same problem while integrating these tests (see PR #461). In this case re-runing the test solved the problem, but as it still gives wired errors I would suggest to deactivate this notebook (but not here, I would suggest to open an extra PR).

Regarding this PR: I will try to have a look unitll end of the week, but I can only test everything on a linux machine. Maybe one of our mac users (@sophieaerdker, @LeanderSchlegel, @reichherzerp) could do some testing.

JulienDoerner commented 5 months ago

For the issue with the failing example test see #467.

rafaelab commented 5 months ago

Thanks for the review, @JulienDoerner I think I've addressed most of the issues you've raised so far and after a long day of attempts I managed to get crpropa-example-test to run, to some extent.

I tried to make the installation default to Python_SITE_PACKAGES (actually, Python_SITELIB) only if the user is not trying to do a local installation using CMAKE_INSTALL_PREFIX. This way, we don't affect the previous behaviour too much, and people like myself who prefer to choose the installation paths are satisfied. What do you think?

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.

lukasmerten commented 4 months ago

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: The tests, however, are all passing for me. So the lensing bug seems to be fixed with this PR.

JulienDoerner commented 4 months ago

Hey @rafaelab,

I think adding this variable Python_INSTALL_PACKAGE_DIR is a good idea. But for me I could not change (or see) it using ccmake. It only appers in the printed log.

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 PREFIX/crpropa, shared data PREFIX/share/crpropa, lib PREFIX/lib/libcrpropa.so and include PREFIX\include\CRPropa.h). I would think the best way would be to allow for the user defined path but default it to PREFIX/lib/python3.X/crpropa. In this case the installation procedure would stay the same as in our installation documentation.

rafaelab commented 4 months ago

Thanks for the feedback, @JulienDoerner and @lukasmerten. Now the flag Python_INSTALL_PACKAGE_DIR is completely independent from CMAKE_INSTALL_PREFIX.

The remaining issue is what @JulienDoerner said about $PREFIX/lib/python3.X/crpropa. This is the internal structure of python packages. But in this case, the user explicitly wants to control all paths, so I fail to see a reason to keep it this way. Why add so many unnecessary layers when we can just have $Python_INSTALL_PACKAGE_DIR directly (at the level of $PREFIX or perhaps $Python_INSTALL_PACKAGE_DIR/python?

JulienDoerner commented 4 months ago

hey @rafaelab,

I think the change is good, as it is. My point for the python3.X/site-packages/ was only about the default value for the $Python_INSTALL_PACKAGE_DIR. But I agree that installing it directly in this level is the best solution. And defaulting it into the Python_SITELIB will keep the previous installation behaviour.

lukasmerten commented 4 months ago

Thanks for your effort @rafaelab. Seems to work for me and I will approve this later.

Just for my records: You can set the flag for the python installation path with ($~/build) cmake -DPython_INSTALL_PACKAGE_DIR=..

Just for curiosity: Is there a way to add Python_INSTALL_PACKAGE_DIR to the parameters that are listed when I run ccmake?

rafaelab commented 4 months ago

Just for my records: You can set the flag for the python installation path with ($~/build) cmake -DPython_INSTALL_PACKAGE_DIR=..

Just for curiosity: Is there a way to add Python_INSTALL_PACKAGE_DIR to the parameters that are listed when I run ccmake?

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. In any case, I will keep this on my to-do list for next time I make general changes to CRPropa.