Skip to content
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

NiftyPET version 2 #1292

Closed
wants to merge 14 commits into from
Closed

Conversation

KrisThielemans
Copy link
Collaborator

upgraded to version 2.

Also added CUDA and NiftyPET to GitHub Actions.

@gschramm @casperdcl @pjmark please check

@KrisThielemans KrisThielemans added this to the v6.0 milestone Nov 25, 2023
@KrisThielemans KrisThielemans force-pushed the NiftyPET2 branch 2 times, most recently from 31d6e01 to 4dfb6c2 Compare November 25, 2023 18:46
@KrisThielemans KrisThielemans force-pushed the NiftyPET2 branch 3 times, most recently from 11ad9a4 to 1c3cba5 Compare November 25, 2023 18:50
- use CUDA version
- let CMake find NiftyPET
- change step condition
- see where NiftyPET*cmake is
- exclude CUDA tests from ctest
@KrisThielemans
Copy link
Collaborator Author

I've got the non-CUDA jobs to run again (phew) (minor correction to come). Turns out that github actions conditional steps are a bit fragile, or I misunderstand the doc, anyway.

I still have a problem letting CMake find NiftyPET (after installation from PyPy). I followed recommendations from
https://github.com/NiftyPET/NIPET/blob/master/README.rst.:

install

             export PATHTOOLS=${GITHUB_WORKSPACE}/NiftyPET_tools
             export HMUDIR=${GITHUB_WORKSPACE}/mmr_hardwareumaps
             python -m pip install "nipet>=2"
             niftypet_cmake_prefix=$(python -c "from niftypet.nipet import cmake_prefix; print(cmake_prefix)")
             ls -R "$niftypet_cmake_prefix"
             echo NiftyPET_PREFIX="$niftypet_cmake_prefix" >> $GITHUB_ENV

gives

niftypet_cmake_prefix=/home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/cmake
ls -R /home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/cmake
/home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/cmake:
NiftyPETmmr_auxeTargets-release.cmake
NiftyPETmmr_auxeTargets.cmake
NiftyPETmmr_lmprocTargets-release.cmake
NiftyPETmmr_lmprocTargets.cmake
NiftyPETnifty_scatterTargets-release.cmake
NiftyPETnifty_scatterTargets.cmake
NiftyPETnipetConfig.cmake
NiftyPETnipetConfigVersion.cmake
NiftyPETpetprjTargets-release.cmake
NiftyPETpetprjTargets.cmake

which surprised me a bit as that doesn't seem to be the prefix, but the actual location of the config files.

Using CMAKE_PREFIX_DIR leads to

cmake -S .. <snip> -DCMAKE_PREFIX_DIR=/home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/cmake <snip>

CMake isn't happy:

By not providing "FindNiftyPETnipet.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "NiftyPETnipet", but CMake did not find one.

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

    NiftyPETnipetConfig.cmake
    niftypetnipet-config.cmake

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

so it seems we need to set NiftyPETnipet_DIR instead. NiftyPET doc bug?

@KrisThielemans
Copy link
Collaborator Author

forgot to mention @casperdcl in the above.

@KrisThielemans
Copy link
Collaborator Author

setting NiftyPETnipet_DIR works fine.

Currently struggling with g++ not finding prbj.h

@KrisThielemans
Copy link
Collaborator Author

I cannot find prbj.h: https://github.com/UCL/STIR/actions/runs/6992200721/job/19023543893#step:6:773

niftypet_cmake_prefix=/home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/cmake
find /home/runner/work/STIR/STIR/my-env/ -name prjb.h

returns nothing

https://github.com/UCL/STIR/actions/runs/6992200721/job/19023543893#step:9:978 shows that the include path set by CMake is likely correct

/usr/bin/g++-12 -DITK_IO_FACTORY_REGISTER_MANAGER -I/home/runner/work/STIR/STIR/src/include -I/home/runner/work/STIR/STIR/build/src/include -I/home/runner/work/STIR/STIR/build/src/ITKFactoryRegistration -I/home/runner/work/STIR/STIR/build/ITKFactoryRegistration -isystem /usr/include/hdf5/serial -isystem /usr/include/gdcm-3.0 -isystem /usr/include/ITK-5.2
 -isystem /home/runner/work/STIR/STIR/my-env/lib/python3.10/site-packages/niftypet/nipet/include -isystem /usr/local/cuda-12.1/include -isystem /home/runner/work/STIR/STIR/install/include -O3 -DNDEBUG -ffast-math -std=gnu++14 -fPIC   -Wall -Wno-deprecated -fopenmp -fopenmp -MD -MT src/recon_buildblock/CMakeFiles/recon_buildblock.dir/NiftyPET_projector/NiftyPETHelper.cxx.o -MF CMakeFiles/recon_buildblock.dir/NiftyPET_projector/NiftyPETHelper.cxx.o.d -o CMakeFiles/recon_buildblock.dir/NiftyPET_projector/NiftyPETHelper.cxx.o -c /home/runner/work/STIR/STIR/src/recon_buildblock/NiftyPET_projector/NiftyPETHelper.cxx
/home/runner/work/STIR/STIR/src/recon_buildblock/NiftyPET_projector/NiftyPETHelper.cxx:41:10: fatal error: prjb.h: No such file or directory

NiftyPET installation bug?

@KrisThielemans
Copy link
Collaborator Author

@casperdcl @pjmark I created NiftyPET/NIPET#51.

I suppose this might still work with a self-installed NIPET, but I have run out of steam.

some members were removed from Cnts
@KrisThielemans
Copy link
Collaborator Author

As NiftyPET 2 is not compatible with C++, I will essentially abandon this PR, only keeping the CI cuda stuff

@pjmark
Copy link

pjmark commented Dec 14, 2023

the core of NIPET is written in C, so should be easy to add it to C++, no?

@KrisThielemans
Copy link
Collaborator Author

The problem is that all the set-up is in Python (where it used to be in C). See NiftyPET/NIPET#52 for more information.

@KrisThielemans
Copy link
Collaborator Author

Non-NiftyPET commits are now in #1300. I'm abandoning the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants