Skip to content

Stir subissue mentioned in SIRF #1357#1690

Open
Dimitra-Kyriakopoulou wants to merge 6 commits intoUCL:masterfrom
Dimitra-Kyriakopoulou:STIR_Subissue_mentioned_in_SIRF_1357
Open

Stir subissue mentioned in SIRF #1357#1690
Dimitra-Kyriakopoulou wants to merge 6 commits intoUCL:masterfrom
Dimitra-Kyriakopoulou:STIR_Subissue_mentioned_in_SIRF_1357

Conversation

@Dimitra-Kyriakopoulou
Copy link
Contributor

Changes in this pull request

  • SIRF #1357 exposed a Python/NumPy algebra problem, and while debugging it, a separate STIR contiguity problem was also noticed.
  • This STIR contiguity problem is fixed here.

Testing performed

  1. STIR unit test
  2. Rebuilt local SIRF wrapper validation

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 (if applicable)
  • The code builds and runs on my machine
  • [] documentation/release_XXX.md has been updated with any functionality change (if applicable)

Contribution Notes

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.
  • I (or my institution) have signed the STIR Contribution License Agreement (not required for small changes).

@Dimitra-Kyriakopoulou
Copy link
Contributor Author

Dear Professor @KrisThielemans ,

  1. In SIRF remove forward_projector from distributable_sensitivity_computation #1357 a separate STIR-side contiguity problem was found:
  • SIRF only allows asarray(copy=False) when the underlying STIR image storage is contiguous
  • for images created from projection metadata, STIR often produced non-contiguous storage
  • because of that, SIRF reported:
    • supports_array_view == False
    • asarray(copy=False) failed

The affected paths that were reproduced were:

  • pet.ImageData(acq)
  • acq.create_uniform_image(1.0)

Before the fix, the usual workaround was:

  • img = img + 0

because that forced a new contiguous copy.

  1. The root cause was traced into STIR C++:
  • file: src/buildblock/VoxelsOnCartesianGrid.cxx
  • function: VoxelsOnCartesianGrid::construct_from_projdata_info(...)

That constructor path ended with:

this->grow(range);

In this code path, using grow(range) goes through resize(), and in practice this was breaking contiguity for these
constructors.

So the constructor path that creates images from ProjDataInfo was allocating in a way that could destroy contiguity.

  1. The fix was to keep this constructor path on STIR's contiguous allocation route.

Instead of:

this->grow(range);

the code now uses:

Array<3, elemT>::operator=(Array<3, elemT>(range));

This matters because:

  • Array(range) allocates a single contiguous block
  • assigning from that freshly created array preserves the intended image range
  • the resulting voxel grid remains contiguous

So the fix is small, but it targets the correct memory-allocation path.

  1. Files Changed In The STIR Fix

Only these 2 STIR files belong to the fix:

  1. src/buildblock/VoxelsOnCartesianGrid.cxx
  2. src/test/test_VoxelsOnCartesianGrid.cxx

Diff summary:

  • VoxelsOnCartesianGrid.cxx: replace the non-contiguous grow(range) path
  • test_VoxelsOnCartesianGrid.cxx: add regression checks that ProjDataInfo-based constructors produce contiguous images
  1. Tests:
  1. STIR unit test

The existing STIR test file was extended:

  • src/test/test_VoxelsOnCartesianGrid.cxx

Added checks:

  • default ProjDataInfo constructor gives is_contiguous() == true
  • sized ProjDataInfo constructor gives is_contiguous() == true
  • multi-zoom ProjDataInfo constructor gives is_contiguous() == true

The rebuilt test ran successfully:

  • result: All tests ok !
  1. Rebuilt local SIRF wrapper validation

To make sure this was not just a STIR-only unit-test result, a local SIRF wrapper was rebuilt against the patched STIR package.

Then the original practical repro was rerun:

  • ImageData(acq)
  • acq.create_uniform_image(1.0)

Final local result:

  • supports_array_view == True
  • asarray(copy=False) succeeds directly

THANK YOU WHOLEHEARTEDLY!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant