Add cil #154

paskino · 2018-11-16T15:14:55Z

Adds CIL optimisation and regularisation.

Adds new iterative reconstruction algorithms from CIL.
Currently tested with SPDHG.

Functionality requires:

Adds CCPi-Framework and CCPi-RGL requires additional packages: cython

KrisThielemans · 2018-11-18T13:14:48Z

version_config.cmake

 else()
-  set(DEFAULT_SIRF_TAG v1.1.1)
+  set(DEFAULT_SIRF_TAG origin/spdhg_from_cil)


can't merge with this

Clearly not. It needs merging of SyneRBI/SIRF#237 and SyneRBI/SIRF#238 and then it'll go away.

This PR required changes in more or less 3 different repos. I wanted to make a working PR for this one. Once we start merging the other PRs this will be cleaned.

KrisThielemans · 2018-11-18T13:15:40Z

SuperBuild/External_CCPi-RGL.cmake

+      set(PYTHON_DEST "${CMAKE_INSTALL_PREFIX}/python")
+    endif()
+    message(STATUS "Python libraries found")
+    message(STATUS "SIRF Python modules will be installed in " ${PYTHON_DEST})


copy-paste mistake

What do you refer to?

SIRF Python module -> CIL?

CMake/FindCython.cmake

SuperBuild.cmake

SuperBuild/External_CCPi-Framework.cmake

KrisThielemans · 2018-11-18T13:19:31Z

SuperBuild/External_CCPi-FrameworkPlugins.cmake

+# Author: Kris Thielemans
+# Author: Edoardo Pasca
+# Copyright 2017 University College London
+# Copyright 2017 STFC


KrisThielemans · 2018-11-18T13:19:44Z

SuperBuild/External_CCPi-RGL.cmake

+# Author: Kris Thielemans
+# Author: Edoardo Pasca
+# Copyright 2017 University College London
+# Copyright 2017 STFC


mehrhardt

I don't think I can contribute to this piece of code. @KrisThielemans and @evgueni-ovtchinnikov should do this.

KrisThielemans · 2018-12-04T22:04:12Z

a note on copyright dates and notices. If you copied code from somewhere, you need to preserve its copyright data/notice. At the same time, you need to make sure it's up to date with your changes.

Seems therefore that you either remove Ben and me, or you keep the original date. sorry

casperdcl · 2019-10-02T20:49:04Z

interesting looks like travis has an old version of docker which is failing to build because of something like GoogleContainerTools/skaffold#2576 - or perhaps the cache was cleared in a previous build which failed.

KrisThielemans · 2019-10-04T10:51:58Z

@casperdcl ready to merge?

casperdcl · 2019-10-04T13:02:27Z

@KrisThielemans there's still https://github.com/CCPPETMR/SIRF-SuperBuild/pull/154/files#r330539512 I think needs to be addressed (or ignored - idk)

casperdcl · 2019-10-04T13:05:03Z

apart from that there's all the hardcoded version/branch stuff which may have to change.

KrisThielemans

A few more very small changes please.

version_config.cmake

CMake/FindCython.cmake

KrisThielemans · 2019-10-05T23:19:15Z

SuperBuild/External_NIFTYREG.cmake

@@ -65,7 +65,8 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
      -DCMAKE_INSTALL_PREFIX=${${proj}_Install_Dir}
      -DUSE_THROW_EXCEP=ON
      # fixes lib_reg_maths.a `GOMP_parallel' undefined reference linker errors
-      -DUSE_OPENMP:BOOL=OFF
+      -DUSE_OPENMP:BOOL=ON


I guess this will break Apple rather dramatically. @rijobro ?

In any case, we need ${proj}_ENABLE_OPENMP as in https://github.com/CCPPETMR/SIRF-SuperBuild/blob/7a479cfd0b936da129e638932ca8b039f714791e/SuperBuild/External_STIR.cmake#L55-L60. To clean that up, we could have a ENABLE_OPENMP option in SuperBuild.cmake, and use its value in the various projects. But feel free to do that with a latter PR (in which case you'd need to raise an issue).

opened #269

SuperBuild/External_TomoPhantom.cmake

KrisThielemans · 2019-10-05T23:21:47Z

docker/user_sirf-ubuntu.sh

@@ -3,7 +3,7 @@
 set -ev
 INSTALL_DIR="${1:-/opt}"
 # SIRF
-git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b master $INSTALL_DIR/SIRF-SuperBuild
+git clone https://github.com/CCPPETMR/SIRF-SuperBuild --recursive -b add_cil $INSTALL_DIR/SIRF-SuperBuild


don't forget!

version_config.cmake

KrisThielemans · 2019-10-05T23:29:25Z

@paskino, @casperdcl please add some lines to CHANGES.md as well before the merge please

casperdcl · 2019-10-08T07:37:14Z

SuperBuild/External_astra-toolbox.cmake

+      PYTHONPATH: prefix PYTHONPATH \n\
+      SETUP_PY:   execute ${PYTHON_EXECUTABLE} setup.py install \n\
+      CONDA:      do nothing")
+    set_property(CACHE PYTHON_STRATEGY PROPERTY STRINGS PYTHONPATH SETUP_PY CONDA)


seems unneeded

casperdcl · 2019-10-08T07:37:57Z

SuperBuild/External_CCPi-Regularisation-Toolkit.cmake

+    set(PYTHON_STRATEGY "PYTHONPATH" CACHE STRING "\
+      PYTHONPATH: prefix PYTHONPATH \n\
+      SETUP_PY:   execute ${PYTHON_EXECUTABLE} setup.py install \n\
+      CONDA:      do nothing")
+    set_property(CACHE PYTHON_STRATEGY PROPERTY STRINGS PYTHONPATH SETUP_PY CONDA)


seems unneeded

casperdcl · 2019-10-08T08:58:37Z

@paskino looks like you squash-merged this which means no git history. @KrisThielemans would you mind if I force-push to master including the history?

casperdcl

post-merge discussions

.travis.yml

casperdcl · 2019-10-08T09:08:35Z

SuperBuild.cmake

@@ -214,6 +216,25 @@ if (BUILD_petmr_rd_tools)
  list(APPEND ${PRIMARY_PROJECT_NAME}_DEPENDENCIES petmr_rd_tools)
 endif()

+if ("${PYTHON_STRATEGY}" STREQUAL "CONDA")
+  set (BUILD_CIL OFF)


@paskino should also set(BUILD_CIL_LITE OFF)?

casperdcl · 2019-10-08T09:10:48Z

SuperBuild.cmake

+option(BUILD_CIL "Build CCPi CIL Modules and ASTRA engine" OFF)
+option(BUILD_CIL_LITE "Build CCPi CIL Modules" OFF)


better to have BUILD_CIL and BUILD_ASTRA separately? Or BUILD_CIL_STRATEGY CACHE STRING "FULL NO_ASTRA"?

KrisThielemans · 2019-10-08T09:45:58Z

Ok. Please send an email to the list Kris

casperdcl · 2019-10-08T10:00:58Z

@KrisThielemans there was a 15min gap between the merge and my force-push, and the code was in the same state so don't think it should be an issue (@paskino you may be the only one who has to git checkout master && git fetch origin && git reset --hard origin/master)

* install linux kernel headers closes SyneRBI#149 * move build-essential at linux-headers install time

paskino added 18 commits July 29, 2018 09:55

added CIL (ccpi-framework) to the dependency

f6910a8

Adds CIL to the SuperBuild

36217fd

Adds CCPi-Framework and CCPi-RGL requires additional packages: cython

uses version_config to set ccpi/cil versions

7f6955b

reflects changes to CCPi-RGL

e54712f

builds latest master

01b41ae

points to spdhg branch

cc47c52

Added CCPi-FrameworkPlugins as CIL dependency

50830ce

updated Gadgetron and ISMRMRD tags

e244b1a

Merge branch 'master' into ubuntu18.04

4205dbc

Merge branch 'master' into ubuntu18.04

a652614

set gadgetron version to 3.17.0

90740d5

fix typo

d23f78c

Merge branch 'master' into add_cil

147a557

fix to CIL commit hash

520c70b

comments removed

6b1ff05

CCPi-Regularisation-Toolkit depends on Cython

2885fbc

removed External_CIL.cmake

fa47308

removed reference to hackathor repo

bb541e4

paskino mentioned this pull request Nov 16, 2018

Add CCPi CIL to SuperBuild SyneRBI/Hackathon-SIRF-SuperBuild#1

Open

paskino requested review from KrisThielemans, evgueni-ovtchinnikov and mehrhardt November 16, 2018 15:20

KrisThielemans requested changes Nov 18, 2018

View reviewed changes

mehrhardt reviewed Nov 19, 2018

View reviewed changes

KrisThielemans added the in-progress label Nov 19, 2018

paskino added 4 commits November 23, 2018 10:30

added date and right package name

c3dacbe

added reference to LICENSE

1f5ad30

added BUILD_CIL option

07acf19

updated date

bc27a0b

safer docker build cache

9f83fe9

casperdcl force-pushed the add_cil branch from 415e6b7 to 9f83fe9 Compare October 2, 2019 21:05

casperdcl added 5 commits October 2, 2019 22:10

fix Edo merge

570937b

Merge remote-tracking branch 'origin/master' into add_cil

ac23054

travis docker cache permissions fix

3d48ff3

try again to fix docker

2d634bf

silly ommission

48bdca8

KrisThielemans requested changes Oct 5, 2019

View reviewed changes

paskino added 8 commits October 7, 2019 11:10

added 2019

51113fd

Gadgetron requires at least Boost 1.65

7fea0a8

swapped master for add_cil branch

bd582aa

removed dependecy from TomoPhantom

213919f

added BUILD_CIL_LITE option

d1808aa

reviewed indentation

d4a99c2

add a BUILD_CIL_LITE=ON

841e808

add test to CIL_LITE

3f97f9a

casperdcl requested changes Oct 8, 2019

View reviewed changes

casperdcl requested review from mehrhardt and removed request for mehrhardt October 8, 2019 07:39

paskino merged commit 4aa0d76 into master Oct 8, 2019

casperdcl reviewed Oct 8, 2019

View reviewed changes

paskino added a commit to paskino/SIRF-SuperBuild that referenced this pull request Oct 25, 2021

install linux kernel headers (SyneRBI#154)

d3ceb67

* install linux kernel headers closes SyneRBI#149 * move build-essential at linux-headers install time

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cil #154

Add cil #154

paskino commented Nov 16, 2018 •

edited

Loading

KrisThielemans Nov 18, 2018

paskino Nov 19, 2018

KrisThielemans Nov 18, 2018

paskino Nov 19, 2018

KrisThielemans Nov 19, 2018

KrisThielemans Nov 18, 2018

KrisThielemans Nov 18, 2018

mehrhardt left a comment

KrisThielemans commented Dec 4, 2018

casperdcl commented Oct 2, 2019

KrisThielemans commented Oct 4, 2019

casperdcl commented Oct 4, 2019

casperdcl commented Oct 4, 2019

KrisThielemans left a comment

KrisThielemans Oct 5, 2019

paskino Oct 7, 2019

KrisThielemans Oct 5, 2019

KrisThielemans commented Oct 5, 2019

casperdcl Oct 8, 2019

casperdcl Oct 8, 2019

casperdcl commented Oct 8, 2019

casperdcl left a comment

casperdcl Oct 8, 2019 •

edited

Loading

casperdcl Oct 8, 2019

KrisThielemans commented Oct 8, 2019 via email •

edited by casperdcl

Loading

casperdcl commented Oct 8, 2019 •

edited

Loading

		option(BUILD_CIL "Build CCPi CIL Modules and ASTRA engine" OFF)
		option(BUILD_CIL_LITE "Build CCPi CIL Modules" OFF)

Add cil #154

Add cil #154

Conversation

paskino commented Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrhardt left a comment

Choose a reason for hiding this comment

KrisThielemans commented Dec 4, 2018

casperdcl commented Oct 2, 2019

KrisThielemans commented Oct 4, 2019

casperdcl commented Oct 4, 2019

casperdcl commented Oct 4, 2019

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Oct 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl commented Oct 8, 2019

casperdcl left a comment

Choose a reason for hiding this comment

casperdcl Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Oct 8, 2019 via email • edited by casperdcl Loading

casperdcl commented Oct 8, 2019 • edited Loading

paskino commented Nov 16, 2018 •

edited

Loading

casperdcl Oct 8, 2019 •

edited

Loading

KrisThielemans commented Oct 8, 2019 via email •

edited by casperdcl

Loading

casperdcl commented Oct 8, 2019 •

edited

Loading