Skip to content

Conversation

@samdporter
Copy link
Contributor

@samdporter samdporter commented May 21, 2025

Changes in this pull request

Add ability to create SPECT subsets with partitioner (as separate ProjData)

Testing performed

None, as yet. Tests require template data (I'll sort this soon)

Related issues

Fixes segfaults arising from using partitioner (SIRF-contribs) on SPECT ProjData

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)

Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 small changes required.

Also, how different is src/test/test_proj_data_info_subsets_spect.cxx from PET? I didn't check, but it's likely most of it is the same. Easy to avoid overlap? (could derive both from one class)

@samdporter
Copy link
Contributor Author

Hmm... this worked on my local machine - segfaults here.

@KrisThielemans
Copy link
Collaborator

The Debug build has the following: https://github.com/UCL/STIR/actions/runs/18319764443/job/52169585055?pr=1596#step:12:231

	Setting up projector pair with ProjMatrixByBinSPECTUB
	Testing Subset forward projection is consistent with reduced segment range
Initial segment range: min_segment_num = 0, max_segment_num = 0
test_proj_data_info_subsets_spect: /home/runner/work/STIR/STIR/src/buildblock/ProjDataInfo.cxx:337: virtual void stir::ProjDataInfo::reduce_segment_range(int, int): Assertion `min_segment_num >= get_min_segment_num()' failed.

@KrisThielemans KrisThielemans self-assigned this Oct 8, 2025
@KrisThielemans
Copy link
Collaborator

Also, how different is src/test/test_proj_data_info_subsets_spect.cxx from PET? I didn't check, but it's likely most of it is the same. Easy to avoid overlap? (could derive both from one class)

I did check. There is some scope for reducing overlap, e.g. by putting the code that explicitly loops over viewgrams to check if they are the same in both PET and SPECT versions etc. However, this will have to remain for later.

@KrisThielemans
Copy link
Collaborator

I've done some minor clean-up. If (when?) tests succeed, I'll squash-merge this. Let me know ASAP if you prefer a normal merge.

@KrisThielemans KrisThielemans added this to the v6.3 milestone Oct 27, 2025
@KrisThielemans KrisThielemans merged commit 54587e7 into UCL:master Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants