GPU gibbs priors refactoring by ColomboMatte0 · Pull Request #1629 · UCL/STIR

ColomboMatte0 · 2025-10-08T15:19:39Z

Main changes:

See presentation:
[https://www.ccpsynerbi.ac.uk/wp-content/uploads/2025/09/GibbsPrior_presentation.pdf]

GibbsPrior.pptx

##Minor changes

added SWIG bindings for the original version CudaRelativedifferencePrior
moved GeneralizedPrior.cxx into GeneralizedPrior.inl
modified src/utilities/stir_timings.cxx to include timings with the new priors (i' ve created also a python notebook used to generate the results of the presentation not included in this pull request, let me know if i should add it (where?) )

Checklist before requesting a review

I have performed a self-review of my code
I have added docstrings/doxygen in line with the guidance in the developer guide
I have implemented unit tests that cover any new or modified functionality (if applicable)
The code builds and runs on my machine
[] documentation/release_XXX.md has been updated with any functionality change (if applicable)

Please tick the following:

[ x] The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in STIR (the Work) under the terms and conditions of the Apache-2.0 License.
[ x] I (or my institution) have signed the STIR Contribution License Agreement (not required for small changes).

- Delete generated CMake and Makefile files - Remove build configuration files - Clean up repository for pull request

…ginal - Remove generated CMake and Makefile build artifacts - Revert QuadraticPrior.cxx to upstream version (removed experimental changes)

- Revert QuadraticPrior.h to upstream version - Revert RelativeDifferencePrior.h to upstream version - Revert CudaRelativeDifferencePrior.cu to upstream version - Revert RelativeDifferencePrior.cxx to upstream version - Remove VS Code workspace file from version control Only keeping new CUDA Gibbs prior implementation files

…tions - Add GibbsRelativeDifferencePrior to recon_buildblock registries - Remove duplicate QuadraticPotential template instantiation - Ensure proper class registration for Python bindings

- Add STIR_WITH_CUDA flag to CMake SWIG flags for proper conditional compilation - Reorganize stir_priors.i to use #ifdef guards for CUDA-specific declarations - Move CUDA shared_ptr declarations and template instantiations inside conditional blocks - Revert utilities/CMakeLists.txt to original (remove test utilities) - Remove personal test file stir_prior_timings.cxx from version control - Clean up template instantiation order and remove duplicate declarations This ensures CUDA priors are only compiled when CUDA support is available, improving build compatibility across different environments.

…ties.cuh for reuse across CUDA modules

deleted vscode workspace file

Removed CUDA Toolkit dependency from CMake configuration.

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

… profiling headers from CudaGibbsPrior.cuh

KrisThielemans · 2025-10-10T08:41:51Z

Appveyor errors with

C:\projects\stir\src\include\stir\recon_buildblock\CUDA\CudaGibbsPrior.h(25,10): error C1083: Cannot open include file: 'cuda_runtime.h': No such file or directory [C:\projects\stir\build\src\swig\_stir.vcxproj]
  (compiling source file 'CMakeFiles/_stir.dir/stirPYTHON_wrap.cxx')
  
Command exited with code 1

so still something wrong with STIR_WITH_CUDA guards or something

KrisThielemans · 2025-10-25T23:36:18Z

@casperdcl can you help with the following? @ColomboMatte0 commited sometimes as root@DESKTOP-U7AVDMQ.
This doesn't feel good, and would likely create difficult with our mailmap. What is the best way to correct this? It'd be nice to preserve history in this PR, but we're going to squash-merge this. First commit was by the root user, so I guess it will the squash-merge will be assigned to root.

I suppose I could do the squash locally, correct the author, then push that to master, then close this PR. Not so great, as github will claim it wasn't merged.

ColomboMatte0 · 2025-10-26T00:30:12Z

i guess there are some instantiation problem for the cpu classes now? amazing!

casperdcl · 2025-10-26T05:58:19Z

You should be able to squash-merge from the UI, and the author will be @ColomboMatte0 as the person who opened the PR - just tidy the squash commit message (remove the "coauthored by root" line at the bottom).

KrisThielemans · 2025-10-26T08:39:50Z

i guess there are some instantiation problem for the cpu classes now? amazing!

no, this isn't instantiation.

In file included from /home/runner/work/STIR/STIR/build/src/swig/CMakeFiles/_stir.dir/stirPYTHON_wrap.cxx:4635:
/home/runner/work/STIR/STIR/src/include/stir/recon_buildblock/GibbsQuadraticPenalty.h:124:9: error: no matching constructor for initialization of 'RegisteredParsingObject<GibbsQuadraticPenalty<float>, GeneralisedPrior<DiscretisedDensity<3, float>>, GibbsPenalty<float, QuadraticPotential<float>>>'
  124 |       : base_type(only_2D, penalisation_factor)
      |         ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/STIR/STIR/build/src/swig/CMakeFiles/_stir.dir/stirPYTHON_wrap.cxx:168950:60: note: in instantiation of member function 'stir::GibbsQuadraticPenalty<float>::GibbsQuadraticPenalty' requested here
 168950 |       result = (stir::GibbsQuadraticPenalty< float > *)new stir::GibbsQuadraticPenalty< float >(arg1,arg2);
        |                                                            ^
/home/runner/work/STIR/STIR/src/include/stir/RegisteredParsingObject.h:77:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
   77 | class RegisteredParsingObject : public Parent
      |       ^~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/STIR/STIR/src/include/stir/RegisteredParsingObject.h:77:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
   77 | class RegisteredParsingObject : public Parent
      |       ^~~~~~~~~~~~~~~~~~~~~~~

The problem is that RegisteredParsingObject<GibbsPenalty,...> doesn't have the constructors of GibbsPenalty. This is likely related to

STIR/src/include/stir/RegisteredParsingObject.h

Lines 80 to 84 in 1f2556b

    
             // import constructors from Parent 
        
             // Note: disabled for older SWIG as that generates an error before 4.2, and a warning for 4.2 
        
           #if !defined(SWIG) || (SWIG_VERSION >= 0x040300) 
        
             using Parent::Parent; 
        
           #endif

I don't know why this only appears now though.

KrisThielemans · 2025-10-26T08:45:09Z

Ubuntu jobs fail, and are running SWIG 4.2.0. MacOS and AppVeyor jobs are fine, and are running SWIG 4.4.0.

KrisThielemans · 2025-10-26T08:52:43Z

I don't know why this only appears now though.

Looks like we might not have run the Actions workflow recentl (as I need to approve it)

ColomboMatte0 · 2025-10-26T09:07:03Z

Ubuntu jobs fail, and are running SWIG 4.2.0. MacOS and AppVeyor jobs are fine, and are running SWIG 4.4.0.

I guess it's related to having removed the .cxx and .cu. The constructors were working fine back there.

KrisThielemans · 2025-10-26T09:44:16Z

The problem is the "overloading" of the SWIG preprocessor definition in STIR, see #1646.

KrisThielemans · 2025-10-26T10:17:25Z

The SWIG preprocessor thing is a bit horrible. As a test, I think replacing SWIG with IGNORESWIG in RegisteredParsingObject.h

#if !defined(SWIG) || (SWIG_VERSION >= 0x040300)
  using Parent::Parent;
#endif

might work, but I need to find something better

KrisThielemans · 2025-10-26T13:14:43Z

Ubuntu jobs fail, and are running SWIG 4.2.0. MacOS and AppVeyor jobs are fine, and are running SWIG 4.4.0.

I guess it's related to having removed the .cxx and .cu. The constructors were working fine back there.

yes, before the definition of the constructor was in the .cxx and .cu, which meant that the wrapper just linked to it. Now it's in the .h, which means that they also need to be compiled when compiling the wrapper .

In any case, having a class definition that changes depending on preprocessor options seems like a recipe for disaster. The only place where it should be safe is for the typedef problem mentioned in #1646

ColomboMatte0 · 2025-10-26T14:28:48Z

The test you suggestet is working, but now i get another error on the MacOS job on recon_testpack related to SRT2D, which i think belong to another PR.Have i done something wrong?

KrisThielemans · 2025-10-26T14:57:53Z

MacOS clang 21 failure due to #1420, all the rest got through! So no, you didn't do anything wrong :-)

KrisThielemans · 2025-10-26T16:20:53Z

@ColomboMatte0 I've created #1647. Let's see...

No longer use `#ifdef SWIG` to change the class definition when compiling the wrapper, but introduce a new preprocessor symbol STIR_COMPILING_SWIG_WRAPPER. This should be used with care! (currently only for typedef etc).

now that we no longer compile the SWIG-generated wrapper with -DSWIG, go back to original version

KrisThielemans · 2025-10-26T20:59:18Z

I've merged #1647 and updated this PR accordingly. Fingers crossed.

ColomboMatte0 · 2025-10-27T14:20:53Z

I've merged #1647 and updated this PR accordingly. Fingers crossed.

Everything worked right?

KrisThielemans · 2025-10-27T15:19:38Z

celebration time!

root and others added 21 commits July 25, 2025 12:05

Add new Gibbs prior files

ac7efc9

Save current progress

2a5afdf

final commit

a169d87

rebased on master

4eab2c5

rebase- FactoryRegister

fa83bd9

Remove build artifacts from src/src directory

39955b5

- Delete generated CMake and Makefile files - Remove build configuration files - Clean up repository for pull request

Clean up: remove build artifacts and revert QuadraticPrior.cxx to ori…

af85e88

…ginal - Remove generated CMake and Makefile build artifacts - Revert QuadraticPrior.cxx to upstream version (removed experimental changes)

Register GibbsRelativeDifferencePrior and clean up template instantia…

8f7fea1

…tions - Add GibbsRelativeDifferencePrior to recon_buildblock registries - Remove duplicate QuadraticPotential template instantiation - Ensure proper class registration for Python bindings

Move blockReduction and atomicAddGeneric functions to stir/cuda_utili…

53e1048

…ties.cuh for reuse across CUDA modules

removed includes for profiling GPU code

5ffa10b

fixed slow derivative10 in the potential function

3699127

Merge remote-tracking branch 'upstream/master' into GPU_gibbs_priors

b788321

Remove utilities/stir_prior_timings.cxx from version control

33967bc

Added Doxygen documentation

86c08ed

removed comment from cudarelativedifferenceprior

15114ef

Delete src/include/stir/recon_buildblock/recon_buildblock.code-workspace

f6ce4c3

deleted vscode workspace file

Remove stir_prior_timings.cxx from CMakeLists

b2c71d2

Delete src/utilities/test_parsing.par

6b326cc

Remove CUDAToolkit find and include directive

0409c8f

Removed CUDA Toolkit dependency from CMake configuration.

KrisThielemans reviewed Oct 9, 2025

View reviewed changes

Comment thread src/include/stir/cuda_utilities.cuh Outdated

KrisThielemans reviewed Oct 9, 2025

View reviewed changes

Comment thread src/utilities/stir_timings.cxx Outdated

ColomboMatte0 added 2 commits October 9, 2025 11:40

Add host and device macros for CUDA compatibility

c58dbf8

Add CUDA compatibility definitions in header file

c90db73

KrisThielemans reviewed Oct 9, 2025

View reviewed changes

Comment thread src/include/stir/recon_buildblock/GibbsRelativeDifferencePrior.h Outdated

ColomboMatte0 and others added 2 commits October 9, 2025 12:57

Update cuda_utilities.cuh with new comment on atomicCAS

32b109b

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

modified stir_timings.cxx methods to avoid compiler warnings, removed…

8bf5861

… profiling headers from CudaGibbsPrior.cuh

KrisThielemans requested changes Oct 10, 2025

View reviewed changes

KrisThielemans requested changes Oct 25, 2025

View reviewed changes

KrisThielemans and others added 2 commits October 26, 2025 00:14

adding a few more doxygen groups

2d17d85

minor doxygen improvements

483d988

KrisThielemans approved these changes Oct 25, 2025

View reviewed changes

SWIG -> IGNORESWIG in RegisteredParsingObject.h

26f2b80

KrisThielemans added 3 commits October 26, 2025 20:51

[SWIG] Change use of STIR preprocessor symbol

5f1c84e

No longer use `#ifdef SWIG` to change the class definition when compiling the wrapper, but introduce a new preprocessor symbol STIR_COMPILING_SWIG_WRAPPER. This should be used with care! (currently only for typedef etc).

updated release notes [ci skip]

61de328

rename IGNORESWIG to SWIG

8565d00

now that we no longer compile the SWIG-generated wrapper with -DSWIG, go back to original version

ColomboMatte0 added 2 commits October 27, 2025 09:56

[ci skip] fix documentation

a942283

[ci skip] typo

692b4d8

KrisThielemans added this to the v6.3 milestone Oct 27, 2025

KrisThielemans added the enhancement label Oct 27, 2025

KrisThielemans merged commit f2e1ca3 into UCL:master Oct 27, 2025

ColomboMatte0 deleted the GPU_gibbs_priors branch October 28, 2025 12:38

Conversation

ColomboMatte0 commented Oct 8, 2025

Main changes:

Checklist before requesting a review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Oct 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Oct 25, 2025

Uh oh!

ColomboMatte0 commented Oct 26, 2025

Uh oh!

casperdcl commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

ColomboMatte0 commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

ColomboMatte0 commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

KrisThielemans commented Oct 26, 2025

Uh oh!

ColomboMatte0 commented Oct 27, 2025

Uh oh!

KrisThielemans commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ColomboMatte0 commented Oct 26, 2025 •

edited

Loading

KrisThielemans commented Oct 26, 2025 •

edited

Loading

KrisThielemans commented Oct 26, 2025 •

edited

Loading