Hackathon Project 8: Expose CUDA-managed STIR image and acquisition queries in sirf.STIR by Dimitra-Kyriakopoulou · Pull Request #1381 · SyneRBI/SIRF

Dimitra-Kyriakopoulou · 2026-03-13T19:44:02Z

This PR is PART of Hackathon Project 8: update STIR data to CUDA managed pointers, then adapt sirf.STIR data-containers and expose the underlying CUDA managed pointer to Python. This PR contains the full SIRF side of that work across Stage 1, Stage 2, and Stage 3.

Stage 1 covers CUDA-managed STIR image queries and Python exposure for the image path.
Stage 2 extends the same path to acquisition/projection data.
Stage 3 uses these SIRF changes together with the companion STIR changes in the end-to-end managed-memory reconstruction proof.

The STIR image part has already been merged upstream in UCL/STIR#1693 (Stage 1) and UCL/STIR#1694 (Stage 2 and Stage 3). The runnable benchmark/proof repository for the whole Project 8 path, including Stage 1, Stage 2, and Stage 3, is Dimitra-Kyriakopoulou/hackathon_project_08_stir_cuda_managed_pointers.

Changes in this pull request

Added image-level CUDA-managed-memory queries in cSTIR.
Added acquisition-level CUDA-managed-memory queries in cSTIR.
Added Python exposure in sirf.STIR for both managed-image and managed-acquisition paths.
The Python-facing query interface now exposes, for the relevant managed STIR objects:
- is_cuda_managed
- cuda_address
- __cuda_array_interface__
For the image path, this means Python can detect whether the image is CUDA-managed, retrieve its underlying address, and obtain a CUDA-array-style view.
For the acquisition path, the same query/exposure pattern is now available for managed acquisition data.
Added the acquisition-side query exposure in cstir_p.cpp.
Kept the Stage 1 image-query path and extended the same design through
Stage 2 and Stage 3, rather than introducing a separate parallel interface.
Propagated STIR_WITH_CUDA into cstir and linked CUDA::cudart, so the CUDA-aware query path is actually compiled in the relevant target.
Scope now covers the full SIRF Project 8 path needed for Stage 1, Stage 2, and Stage 3.

Testing performed

Validated the C++ SIRF image probe end to end.
Validated Python import of sirf.STIR with the managed-image factory.
Checked the Stage 1 image-side Python properties on a real managed image:
- is_cuda_managed
- cuda_address
- __cuda_array_interface__
Ran the Stage 1 phantom-clone benchmark; the managed image matched the source phantom exactly.
Validated the Stage 2 deterministic acquisition benchmark.
Checked the Stage 2 acquisition-side Python properties on real managed acquisition data:
- supports_array_view = true
- is_cuda_managed = true
- cuda_address = nonzero
Validated the Stage 3 reconstruction proof:
- managed acquisition input remained managed
- managed image output remained managed
- the CUDA address of the managed output did not change across the reconstruction update
- managed and normal reconstructions matched to machine precision
The runnable proof-of-use benchmarks for Stage 1, Stage 2, and Stage 3 are kept separately in Dimitra-Kyriakopoulou/hackathon_project_08_stir_cuda_managed_pointers.

Related issues

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
[x ] The code builds and runs on my machine
CHANGES.md has been updated with any functionality change

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

[x ] The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.

KrisThielemans

I think we need to move some things to DataContainer, in particular . Not so sure why we didn't do that yet with address() itself.

I suggest to rename is_cuda_managed to supports_cuda_array_view to be consistent with existing code. We should have a virtual DataContainer version that just returns false.

I'm not so sure we need cuda_address() as it's really the same as address() but with a check. If we do have it, it should be in DataContainer (calling virtual DataContainer::address()), and it shouldn't be anywhere else.

KrisThielemans · 2026-03-14T09:07:13Z

.github/workflows/build-test.yml

 on:
  push:
-    branches: [ master ]
+    branches: [ master, Hackathon_project8_II ] 


will need to undo

.github/workflows/build-test.yml

src/xSTIR/cSTIR/include/sirf/STIR/stir_data_containers.h

src/xSTIR/cSTIR/CMakeLists.txt

src/xSTIR/cSTIR/stir_data_containers.cpp

src/xSTIR/pSTIR/STIR.py

KrisThielemans · 2026-03-14T10:47:26Z

src/xSTIR/pSTIR/STIR.py

+
+    @property
+    def __cuda_array_interface__(self):
+        """As per https://numba.readthedocs.io/en/stable/cuda/cuda_array_interface.html"""


apparently that site is deprecated.

Suggested change

"""As per https://numba.readthedocs.io/en/stable/cuda/cuda_array_interface.html"""

"""As per https://nvidia.github.io/numba-cuda/user/cuda_array_interface.html"""

resolved, but not changed?

Dimitra-Kyriakopoulou · 2026-03-14T11:12:08Z

Dear Professor @KrisThielemans,
THANK YOU WHOLEHEARTEDLY!!! I have just closed SyneRBI/SIRF#1380, and I will now work on your comments.

Dimitra-Kyriakopoulou · 2026-03-14T16:54:35Z

Dear Professor @KrisThielemans ,
THANK YOU WHOLEHEARTEDLY FOR YOUR REVIEW!!!

I applied the comments, and replied. In particular, for

I think we need to move some things to DataContainer, in particular . Not so sure why we didn't do that yet with address() itself.

I suggest to rename is_cuda_managed to supports_cuda_array_view to be consistent with existing code. We should have a virtual DataContainer version that just returns false.

I'm not so sure we need cuda_address() as it's really the same as address() but with a check. If we do have it, it should be in DataContainer (calling virtual DataContainer::address()), and it shouldn't be anywhere else.

I moved the CUDA-array-view support to the generic DataContainer layer, including the common C++/C/Python path, and renamed it to supports_cuda_array_view. cuda_address() is now only provided generically via DataContainer::address().

I had also forgotten copyright and author metadata.

I updated the existing UCL/UCL-style copyright lines to include 2026 (for you), and added Copyright 2026 Biomedical Research Foundation, Academy of Athens (for me).
I did this according to the existing conventions of the files: I kept author metadata (both for you and me) only where the files already had author metadata, plus the new Common.h; I did not add author lines to the Python files or to older C/C++ files that historically had none.
I will need to add copyright/author (both for you and me) to the STIR files too: hence i will take the opportunity to add the references for the original papers of GRD2D and DDSR2D

Please inform me if any of the changes are not OK, and i will very gladly attempt to correct them.

THANK YOU WHOLEHEARTEDLY!!!
dimitra

KrisThielemans

Thanks!

Let's move the pointer functions to common/utilities.h (sorry).

Also, all address() functions in the derived classes will now need override. And add something to CHANGES.md along the lines of "add supports_cuda_array_view() and related functions to expose CUDA arrays directly to Python for DataContainers that support it."

KrisThielemans · 2026-03-14T19:46:07Z

src/xSTIR/cSTIR/stir_data_containers.cpp

+bool
+STIRAcquisitionDataInMemory::supports_cuda_array_view() const
+{
+	return this->supports_array_view() && pointer_supports_cuda_array_view(reinterpret_cast<const void*>(this->address()));


I think this is a generic implementation that can be used in DataContainer, and we then won't need it anywhere else

Done: moved to the generic DataContainer/common path and removed the STIR-specific implementation.

KrisThielemans · 2026-03-14T19:47:15Z

src/xSTIR/pSTIR/STIR.py

+
+    @property
+    def __cuda_array_interface__(self):
+        """As per https://numba.readthedocs.io/en/stable/cuda/cuda_array_interface.html"""


resolved, but not changed?

KrisThielemans · 2026-03-14T19:49:37Z

src/xSTIR/cSTIR/CMakeLists.txt

+if (TARGET CUDA::cudart)
+  get_target_property(_cudart_include_dirs CUDA::cudart INTERFACE_INCLUDE_DIRECTORIES)
+  set(_saved_required_includes "${CMAKE_REQUIRED_INCLUDES}")
+  if (_cudart_include_dirs)
+    set(CMAKE_REQUIRED_INCLUDES ${_cudart_include_dirs})
+  endif()
+  check_include_file_cxx(cuda_runtime_api.h HAS_CUDA_RUNTIME_API)
+  set(CMAKE_REQUIRED_INCLUDES "${_saved_required_includes}")
+  if (HAS_CUDA_RUNTIME_API)
+    target_compile_definitions(cstir PUBLIC HAS_CUDA_RUNTIME_API)
+    target_link_libraries(cstir CUDA::cudart)
+  endif()
+endif()


@casperdcl are these checks needed? i.e. if the target exists, aren't we guaranteed to have cuda_runtime_api.h?

I simplified this by dropping the extra header check and using TARGET CUDA::cudart directly.

KrisThielemans · 2026-03-14T19:57:17Z

src/common/include/sirf/common/Common.h

+\ingroup Common
+
+\author Dimitra Kyriakopoulou
+\author Kris Thielemans


I didn't write any of this, so remove me!

Because your guidance raises points I would not have thought of on my own, I experience this as a collaborative effort and for that reason I regard you as a coauthor.

Dimitra-Kyriakopoulou · 2026-03-14T21:47:15Z

Dear Professor @KrisThielemans ,
THANK YOU WHOLEHEARTEDLY!!!

Let's move the pointer functions to common/utilities.h (sorry).

Moved to src/common/include/sirf/common/utilities.h.

all address() functions in the derived classes will now need override

Done.

add something to CHANGES.md ...

Added.

resolved, but not changed?

This had already been changed; the current code raises AttributeError when unsupported, and the thread appears to be attached to an outdated diff.

I will be looking forward to your further evaluation!

THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!
Dimitra

KrisThielemans

A little bit of clean-up. We have reduced the number of changes, which is always good!

KrisThielemans · 2026-03-14T22:58:35Z

src/common/csirf.cpp

+bool
+DataContainer::supports_cuda_array_view() const
+{
+	return this->supports_array_view() &&
+		pointer_supports_cuda_array_view(reinterpret_cast<const void*>(this->address()));
+}
+


This really doesn't belong in csirf.cpp, which defines the C interface. I think best to create a DataContainer.cpp and put it in there. The alternative would be to add it to the .h file, but then we need to include utilities.h in there, which I'd like to avoid.

Done: I moved the implementation to a new src/common/DataContainer.cpp, so it is no longer in the C interface file and DataContainer.h no longer needs to include utilities.h.

This proved related to the second CI issue (please see post below). The problem there was that I had placed the generic DataContainer::supports_cuda_array_view() implementation in csirf.cpp. I now moved it to src/common/DataContainer.cpp and removed it from src/common/csirf.cpp, so it no longer lives in the C interface file and DataContainer.h still does not need to include utilities.h.

KrisThielemans · 2026-03-14T23:03:38Z

src/Registration/cReg/include/sirf/Reg/NiftiImageData.h

 SyneRBI Synergistic Image Reconstruction Framework (SIRF)
-Copyright 2017 - 2020 University College London
+Copyright 2017 - 2020, 2026 University College London
+Copyright 2026 Biomedical Research Foundation, Academy of Athens


I don't think it makes sense to add copyright info and authorship for just adding override. (also in other places)

Undone here and in the related files where only override/plumbing changes had prompted those metadata edits.

KrisThielemans · 2026-03-14T23:05:37Z

src/xSTIR/cSTIR/CMakeLists.txt

-# Author: Kris Thielemans, Richard Brown
+# Author: Kris Thielemans
+# Author: Richard Brown
+# Author: Dimitra Kyriakopoulou


looks like we should undo these changes

KrisThielemans · 2026-03-14T23:05:59Z

src/xSTIR/cSTIR/stir_data_containers.cpp

 Copyright 2017 - 2023 Rutherford Appleton Laboratory STFC
-Copyright 2018 - 2023 University College London
+Copyright 2018 - 2023, 2026 University College London
+Copyright 2026 Biomedical Research Foundation, Academy of Athens


KrisThielemans · 2026-03-14T23:06:58Z

CHANGES.md


 * SIRF/STIR
  - The implementation of the creation of `sirf.STIR.ImageData` from `sirf.STIR.AcquisitionData` has been revised to ensure compatibility of `ImageData` dimensions and voxel sizes with `AcquisitionData`.
+  - Added `supports_cuda_array_view()` and related `DataContainer`/Python support, including `__cuda_array_interface__`, to expose CUDA-backed arrays directly for supported containers such as STIR image and acquisition data.


Suggested change

- Added `supports_cuda_array_view()` and related `DataContainer`/Python support, including `__cuda_array_interface__`, to expose CUDA-backed arrays directly for supported containers such as STIR image and acquisition data.

- Added `supports_cuda_array_view()` and related `DataContainer`/Python support, including `__cuda_array_interface__`, to expose CUDA-backed arrays directly for supported containers

we have to cut STIR support, as it hasn't been merged yet

Dimitra-Kyriakopoulou · 2026-03-14T23:50:15Z

Dear Professor @KrisThielemans,
THANK YOU WHOLEHEARTEDLY!!!

I faced two separate CI failures locally.

ImageWrap::address() const override

I applied the review request that all derived address() functions should get override too broadly by adding it in
gadgetron_image_wrap.h. However, ImageWrap is not derived from DataContainer/ImageData. I fixed this by
removing override there.

MR_PROCESS_TESTDATA link error with GEHDF5Wrapper / H5::...

Undoing the temporary CI restrictions restored the full build in .github/workflows/build-test.yml. This caused CI to
build the Gadgetron/Synergistic path again, including MR_PROCESS_TESTDATA, which is why this problem became visible.

The likely direct trigger in my implementation was that, in the follow-up review changes, I moved the generic
supports_cuda_array_view() implementation out of DataContainer.h and into csirf.cpp. That implementation choice
appears to have pulled a broader STIR/Synergistic link path into that Gadgetron test.

I am therefore keeping supports_cuda_array_view() generic at the DataContainer level, but moving its
implementation back inline into DataContainer.h instead of defining it out of line in csirf.cpp.

I will be looking forward to your further evaluation!

THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!
Dimitra

Expose CUDA-managed STIR image and acquisition queries

bfbfbde

KrisThielemans mentioned this pull request Mar 14, 2026

Hackathon project8: Expose CUDA-managed STIR image queries in sirf.STIR #1380

Closed

4 tasks

Dimitra-Kyriakopoulou changed the title ~~Hackathon Project 8: Expose CUDA-managed STIR image and acquisition queries in sirf.STIR (continuation of the image stage)~~ Hackathon Project 8: Expose CUDA-managed STIR image and acquisition queries in sirf.STIR Mar 14, 2026

KrisThielemans requested changes Mar 14, 2026

View reviewed changes

Align CUDA array-view support with DataContainer API

1ecb7ad

Dimitra-Kyriakopoulou force-pushed the Hackathon_project8_II branch from 806c38b to 1ecb7ad Compare March 14, 2026 16:17

Suppress cppcheck CUDA include false positive

0e7ff1e

KrisThielemans requested changes Mar 14, 2026

View reviewed changes

KrisThielemans self-assigned this Mar 14, 2026

KrisThielemans added the enhancement label Mar 14, 2026

Generalise CUDA array-view support in DataContainer

ac1badc

Fix override on Gadgetron ImageWrap wrapper

629941a

KrisThielemans requested changes Mar 14, 2026

View reviewed changes

Move generic DataContainer CUDA array-view support out of csirf

a7cb0f4

casperdcl added this to 2026-03 AIRBI Hackathon Mar 16, 2026

casperdcl moved this to In Progress in 2026-03 AIRBI Hackathon Mar 16, 2026

	"""As per https://numba.readthedocs.io/en/stable/cuda/cuda_array_interface.html"""
	"""As per https://nvidia.github.io/numba-cuda/user/cuda_array_interface.html"""

	- Added `supports_cuda_array_view()` and related `DataContainer`/Python support, including `__cuda_array_interface__`, to expose CUDA-backed arrays directly for supported containers such as STIR image and acquisition data.
	- Added `supports_cuda_array_view()` and related `DataContainer`/Python support, including `__cuda_array_interface__`, to expose CUDA-backed arrays directly for supported containers

Conversation

Dimitra-Kyriakopoulou commented Mar 13, 2026 • edited by paskino Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

Contribution Notes

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dimitra-Kyriakopoulou commented Mar 14, 2026

Uh oh!

Dimitra-Kyriakopoulou commented Mar 14, 2026

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dimitra-Kyriakopoulou commented Mar 14, 2026

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dimitra-Kyriakopoulou commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Dimitra-Kyriakopoulou commented Mar 13, 2026 •

edited by paskino

Loading