3d-pli / fastpli Goto Github PK

I am trying to get the testing running on the WSL. I run into 2 problems so far

• The dependencies do not specify minimum version requirements. Some tests were failing for me as np.cross requires numpy>=1.19
• Running the tests a second times was failing at a very preliminary stage as a temporary file /tmp/simply_rep.h5 could not be written as it was already present. Maybe there should be a more gracious handling of temporary file removal if tests fail

In the README, the gi clone line shoud read:

git clone --recursive https://github.com/3d-pli/fastpli.git

The matplotlib 3D module is not very powerfull, I think data visualization for this library would benefit from native 3D libraries.

In the notebook, ipyvolume is quite simple to setup. In a Qt / desktop context, vispy is well established.

Although install proceeded without problems, I had the following warning:

WARNING: Built wheel for fastpli is invalid: Metadata 1.2 mandates PEP 440 version, but 'v1.1.0a6.dev1-g2b24015-Release' is not
Failed to build fastpli
Installing collected packages: numpy, llvmlite, scipy, numba, h5py, fastpli
    Running setup.py install for fastpli ... done
  DEPRECATION: fastpli was installed using the legacy 'setup.py install' method, because a wheel could not be built for it. A possible replacement is to fix the wheel build issue reported above. You can find discussion regarding this at https://github.com/pypa/pip/issues/8368.

I assume it is a simple fix to not put the git hash in the version, but I'm not familiar with the system you're using

This is a very minor problem, please feel free to not fix :)

I have been reading through the wiki and I can give some feedback on unclear points/things that I think might help clarifying stuff for the user. I apologise in advance if I am saying stuff just because I missed pieces, I just want to give fully constructive feedback from a completely external eye! :)

fastPLI is an open source toolbox for modeling nerve fibers, simulating them in a 3D PLI [I would probably have it as Polarized light imaging at least in the package line description] microscope and --the-- [simulation?] signal processing.

I take it is not the case, but I would make it even clearer if it is just for simulation, or also for PLI data analysis (fastpli.analysis is the first module you read about in the README, and you might wonder if you can use the package also to just analyse real world data you might provide).

Probably it would be good to guide the user through a reasonable reading order, either with a sequential list of readings in the home of the user that a person should go through, or a reasonable order of pages on the right. Eg, you might want to have them reading about how FiberBundles are represented as the first step. Moreover, you probably want in the organisation of such reading to split even more clearly everything that is related to the generation of the bundles (the sandbox, collision solving, etc); from the reading you would need to do if you just want to simulate the PLI results from the fibres.

I think that adding some intro lines to every page where you showcase code might be useful (as you do eg in https://github.com/3d-pli/fastpli/wiki/Simulation but not in https://github.com/3d-pli/fastpli/wiki/Crossing).

Finally, for users to start get hands in in the package usage, it would be great if those wiki pages could also be distributed as tutorial jupyter notebooks in the package!

I got an error trying to compile with cmake on Ubuntu 18.04 (Python==3.7.3)

CMake Error at CMakeLists.txt:80 (FIND_PACKAGE):
  By not providing "FindPython3.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Python3", but
  CMake did not find one.

  Could not find a package configuration file provided by "Python3" with any
  of the following names:

    Python3Config.cmake
    python3-config.cmake

  Add the installation prefix of "Python3" to CMAKE_PREFIX_PATH or set
  "Python3_DIR" to a directory containing one of the above files.  If
  "Python3" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/mnt/pcshare/users/rpx/gitpod/fastpli/build/CMakeFiles/CMakeOutput.log".
Makefile:36: recipe for target 'build/Makefile' failed
make: *** [build/Makefile] Error 1

This package would benefit from conda-feedstock for binaries cross-compilation and distribution.

I think openmpi and freeglut can be installed as conda packages directly, so the install recipe should not be too complicated to write.

Just skimming through the project I could find several places in which the usage of linting tools such as flake8 and black would help, eg:

• inconsistent single and double quote usage;
• unused import statements
• unused, commented out code (this should not live in master)

Ideally those can be running together with the CI mentioned in #1 for testing linting together with test passing.

It would be highly recommended if the full documentation of the package is hosted somewhere, on GitHub Pages or ReadTheDocs, instead of having this to be run on the user's side and stored locally (in this case, the sphinx-rtd-theme and the sphinx-autodoc-typehints deps are missing)

I would like to as for clarification as to why a Fiber is not described in Python as its own class, it would probably be better to validate the input data for generating it and ensuring without further checking that all subsequent functions operating on Fibers, FiberBundles etc get consistent input.

Regardless, I was also a bit confused at first that in the module named objects all the transformations are functions, but capitalised as classes should; is there a reason? I have to say that I am still trying to fully understand the interaction between the C++-compiled part of the code, and the Python parts that make use of such modules; if it is related to that, I would be happy to be given additional pointers on the topic. Probably it would be good if a documentation section could also deal with explaining a bit further the C++/Python integration. If there is such file and I missed it apologies, and please link it to me!

Actually if it is in the user generated documentation, see #4

Hi there,
I will start addressing the points that I would like to raise for the JOSS review through issues.

I would start from continuous integration; I found the gitlab-ci.yml file, but I don't see any GitHub workflow in place to ensure test running on at least the master branch. Am I missing something? Maybe I am just not familiar with your CI workflow, but ideally there should be a badge in the README on the testing state of the package!