otvam / pypeec

PyPEEC - 3D Quasi-Magnetostatic FFT/PEEC Solver
https://pypeec.otvam.ch
Mozilla Public License 2.0
15 stars 2 forks source link

[joss review] python requirements ? #5

Closed thelfer closed 6 months ago

thelfer commented 7 months ago

[This is issue is part of the review of the paper submited to JOSS.]

First, I would recommend an INSTALL file at the root of the sources. On the first contact with the library, I didn't see any installation instructions and had to figure out that I had to use pip from the "Project links" section of the README. The installation instructions are somehow hidden in the tutorial, which is is not so obvious. At least the INSTALL file could link to the tutorial, IMHO. Also the root of the directory contains file related to conda, which is not mentioned in the installation section.

Regarding first contact with pypeec, there are a few scripts called run... at the root of the sources which seems tools for developers. The prefix run seems misleading and users may be tempted to have a look at them. That is only an totally discardable advice, but maybe such scripts would be better in a dedicated subdirectory (something like tools for instance).

I followed your installation guide and it failed as follows:

(pypeec-env) $ python3 -m pip install pypeec
Collecting pypeec
  Could not find a version that satisfies the requirement pypeec (from versions: )
No matching distribution found for pypeec
(pypeec-env)$ python3 
Python 3.7.3 (default, Oct 11 2023, 09:51:27) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

I guess that my version of python is too old. Would you tell me some minimal requirements for pypeec ?

otvam commented 7 months ago

Thank you very much for accepting this review and for the comments!

Concerning, the installation instruction, I will add a "INSTALL.md" file at the root of the repository. You are totally right that the "run_*.sh" are maintenance scripts that are only useful for the maintainers, so I will move them to a "scripts" directory.

For the installation, the supported Python version are ["3.9", "3.10", "3.11", "3.12"]. I know that several Linux distributions are still shipping older versions but some libraries used by PyPEEC requires a more recent version.

Probably, conda is the easiest way to create a environment with a compatible Python interpreter:

conda create -n pypeec python=3.10         # create the conda env with Python
conda activate pypeec                      # activate the environment
conda install -c conda-forge libstdcxx-ng  # the env might be missing this system library for the plots
python -m pip install pypeec               # install pypeec and the dependencies
pypeec -v                                  # check the installation
thelfer commented 7 months ago

No problem. Thanks for taking my remarks into account.

For the installation, the supported Python version are ["3.9", "3.10", "3.11", "3.12"]. I know that several Linux distributions are still shipping older versions but some libraries used by PyPEEC requires a more recent version.

This shall be cleary stated in the INSTALL.md file. I did not check yet what is you CI procedure if any, but tested configurations is a very important information for end users (i.e. I would have known that I needed a more recent python version).

Probably, conda is the easiest way to create a environment with a compatible Python interpreter:

Thanks. My point was not to use conda, but that conda's usage was not documented. And I naively though that a conda package was available, i.e. that pip was not the only way of installing PyPEEC.

otvam commented 7 months ago

Yes, ["3.9", "3.10", "3.11", "3.12"] are the versions tested during the CI.

The version requirements are now clearly stated in the INSTALL.md and in the documentation.

The problem to create a conda package is that the VTK (C++ library for 3D object manipulation) conda package is currently broken for win64 (#6). I might have found a workaround and will try to create a Conda package.

thelfer commented 7 months ago

@otvam Thanks for the precision. Concerning conda, don't be mistaken. My remark originates from the fact that there was no INSTALL file, so I had to somehow guess how to install it. I saw files related to conda at the root directory and though "maybe there is a conda package ?". For the review, a conda package is not a matter, having clear install instructions is. So don't bother with a conda package for the review (but feel free to do it if you find it important for the project)

otvam commented 6 months ago

The conda package is now available through conda-forge! It was overdue because the HPC libraries used by PyPEEC are much easier to install with conda.