CUDARelativeDifferencePrior#1408
Conversation
KrisThielemans
left a comment
There was a problem hiding this comment.
Thanks!
There are a lot of warnings with regards to -- virtual function override intended?.
please give an example
The set_up_cuda is a bit clunky (I did have it override the set-up previously but got a bit confused with CRTP things)
This won't help, as STIR will never call set_up_cuda. There's too much CRTP going on here (if any). Do you mean calling the baseclass set_up()?
I think it'd make more sense to use recon_buildblock/CUDA than CUDA_stir (why have stir twice?)
| find_package(CUDAToolkit) | ||
| if (CUDAToolkit_FOUND) | ||
| set(CMAKE_CUDA_ARCHITECTURES "all") | ||
| find_package(CUDA REQUIRED) | ||
| include_directories("${CUDA_INCLUDE_DIRS}") | ||
| set(STIR_WITH_STIR_CUDA ON) | ||
| if(STIR_WITH_STIR_CUDA) | ||
| enable_language(CUDA) | ||
| message(STATUS "STIR CUDA support enabled. Using CUDA version ${CUDAToolkit_VERSION}.") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
I think this is the other way around. I'd probably use https://cmake.org/cmake/help/latest/module/CheckLanguage.html#module:CheckLanguage first, if present, then enable_language, then find_package(CUDAToolkit) which probably needs a REQUIRED.
Before all this, I'd do a check on CMAKE_VERSION. Certainly at least 3.18, but 3.23 if we want to use "all" for [CMAKE_CUDA_ARCHITECTURES](https://cmake.org/cmake/help/latest/prop_tgt/CUDA_ARCHITECTURES.html#prop_tgt:CUDA_ARCHITECTURES) (which we do). If the CMake version is too old, generate FATAL_ERROR with the message to set DISABLE_STIR_CUDA=ON`.
There was a problem hiding this comment.
Also, don't use include_directories but target_include_directories
There was a problem hiding this comment.
It's this line:
if (STIR_WITH_CUDA)
target_include_directories(stir_registries PRIVATE ${CUDA_INCLUDE_DIRS})
endif()
|
|
||
| START_NAMESPACE_STIR | ||
|
|
||
| static void |
There was a problem hiding this comment.
let's not repeat those. We can make them static members of RelativeDifferencePrior or wherever they need to be
There was a problem hiding this comment.
I guess this is with regards to the compute weights @KrisThielemans ?
|
|
ok. Nothing to do with this PR. Would you mind creating a separate issue for it? Please add which compiler you're using. (It's a false positive, but would be nice to have a clean way to prevent the warning) |
I am having some issues getting it to register correctly in the stir registries. Perhaps it's related to that? I am not sure I am correctly making |
| target_include_directories(stir_registries PRIVATE ${STIR_INCLUDE_DIR}) | ||
| target_include_directories(stir_registries PRIVATE ${Boost_INCLUDE_DIR}) | ||
| if (STIR_WITH_CUDA) | ||
| target_include_directories(stir_registries PRIVATE ${CUDA_INCLUDE_DIRS}) |
There was a problem hiding this comment.
I really hope we don't need CUDA_INCLUDE_DIRS for the registries. STIR .h files ideally have no CUDA includes at all. It's going to create trouble.
There was a problem hiding this comment.
I thought we'd want the target to be the registeries as we need the cuda_runtime.h when building cuda files that will be in the registry... Also I couldn't quite figure out where else to put the target_include_directories, where would you suggest?
There was a problem hiding this comment.
As far as I can see,cuda_runtime.h should NOT be included in src/include/stir/recon_buildblock/CUDA/CudaRelativeDifferencePrior.h, but in the .cxx/.cu. That means that the registries wouldn't need to know about the CUDA include dir, therefore you don't need the target_include_directories` at all. (The .cu files need it, but they get it by depending on CUDA::cudart target).
|
|
||
| #include "stir/recon_buildblock/RelativeDifferencePrior.h" | ||
| #include "stir/DiscretisedDensity.h" | ||
| #include <cuda_runtime.h> |
There was a problem hiding this comment.
this is done but dim3 is cuda specific, I changed it for a native struct and removed the cuda_runtime include, aren't the other two needed?
no
Please show errors/problems. I have no time to try this myself. sorry. |
Thanks @KrisThielemans I was running Is the overwriting causing the issue, or I am not sure if I have updated all the neccessary files to register it correctly. I guess part of my concern is whether I updated all the neccessary files so that STIR can include the new class in the registeries. This is a quite new and pretty confusing |
|
Do you have the equivalent of https://github.com/UCL/STIR/blob/8ced2d73933420457e0bc76074964b7d4ff00f0c/src/recon_buildblock/RelativeDifferencePrior.cxx#L140C1-L141C97 |
| // Explicit template instantiations | ||
| template class stir::CudaRelativeDifferencePrior<float>; | ||
| template <typename elemT> | ||
| const char* const CudaRelativeDifferencePrior<elemT>::registered_name = "Cuda Relative Difference Prior"; |
There was a problem hiding this comment.
@KrisThielemans yes I added an overwrite it
There was a problem hiding this comment.
please use exact same syntax as in the lines quoted above. C++ is a bit tricky there. (or try and see what happens if you put the initialiser in the .h file). If that doesn't work, comment out the one in RelativeDifferencePrior, or remove that from the registry completely. I don't think we've ever tried deriving from an existing class in the registry.
There was a problem hiding this comment.
@KrisThielemans is the reason for not deriving from an existing class in the registry, because it's just a bad idea? I couldn't think of a better idea...
There was a problem hiding this comment.
Not sure what you mean. Ideally we can derive from an existing class, but if it doesn't work, we'll have to find another way...
So first thing to do is to fix your syntax and see what happens.
|
@Imraj-Singh any chance you can pick this up? I think my suggestions might resolve your trouble. Also, it'll need a merge with I'd like to you the main CUDA support stuff elsewhere... |
Just back I will spend some time the next couple of days. |
|
@KrisThielemans I am still having troubles with the registering the class derived from an already registered class... The error is below.
I'll try remove the registering in the |
|
@KrisThielemans I tried removing the registering of |
This is because your initialisation of Possible solutions:
If the latter works, it's my preferred solution (by far clearest for the user), and we could do it for the rest of STIR now as well. |
|
Also, please add |
Problems with numerical gradients. I had to revert the change to GeneralisedPrior::set_penalisation_factor which set _already_set_up to false, as that made test_priors fail on all priors (as the test changes the penalisation factor)
|
Note that the test image are very small (8x9x10). Not sure if that matters or not. @Imraj-Singh ? |
- there was a missing epsilon in the gradient kernel - abs was used, which is potentially dangerous, so now using std::abs - switch value back to double, as otherwise test_priors still failed
The Hessian() functions all incorrectly threw an error if kappa was alright. I've now moved this test into the check() functions, cleaning code a little bit.
- tests were never run with kappa - RDP limit tests with kappa were wrong Still numerical problems for the Hessian test for RDP
|
The above problems have now been resolved. 2 things remaining
|
RDP test was failing with kappa as eps was too large. Now it is set relative to the iamge max.
CUDA run-time is not available on GitHub actions, so we need to able to disable them.
|
Everything fine now, except that there's no CUDA run-time on GHA, so we have to disable that test somehow. |
It was only coded for float before. I haven't tested with double though.
|
All good now! (except a Zenodo time-out) |
removed obsolete cuda_rdp_tests accordingly
|
Should be done now. I've added the timings to compared to parallelproj forward proj 225ms and back proj 573ms. Thanks @Imraj-Singh! |

First attempt at integrating CudaRelativeDifferencePrior class into STIR. The idea was create a child of
RelativeDifferencePriorto override thecompute_value/compute_gradientmethods with the CUDA-accelerated counterpart, and then add anset_up_cudamethod. This is in a first-draft state with work left to do:-- virtual function override intended?.At present it does run with a test comparing outputs of CPU/GPU versions. I could include it in the pull-request but I need to spend sometime making the ctest I guess.