-
Notifications
You must be signed in to change notification settings - Fork 95
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
CUDARelativeDifferencePrior #1408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
CMakeLists.txt
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, don't use include_directories
but target_include_directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here: d3f3732
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is with regards to the compute weights @KrisThielemans ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
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 |
src/CMakeLists.txt
Outdated
@@ -244,6 +244,9 @@ add_library(stir_registries OBJECT ${STIR_REGISTRIES}) | |||
# TODO, really should use stir_libs.cmake | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to cxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KrisThielemans yes I added an overwrite it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
No need to compute the value per element as double. it is only the accumulation that where it matters.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done
First attempt at integrating CudaRelativeDifferencePrior class into STIR. The idea was create a child of
RelativeDifferencePrior
to override thecompute_value
/compute_gradient
methods with the CUDA-accelerated counterpart, and then add anset_up_cuda
method. 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.