Skip to content
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

Introduce classes for ProjData indices #1273

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

KrisThielemans
Copy link
Collaborator

@KrisThielemans KrisThielemans commented Oct 15, 2023

This PR introduces SegmentIndices, ViewgramIndices and SinogramIndices (and derives Bin from ViewgramIndices ) and add constructors and methods to use these, as opposed to listing indices separately. This will allow for more concise code, but also prepares for TOF (and energy/layers etc).

Using these new methods is left for the TOF branch.

DataSymmetriesForViewSegmentNumbers etc have not been modified yet.

Fixes #1263

@KrisThielemans
Copy link
Collaborator Author

temp zenodo problems. Also, Appveyor didn't run, as last commit was only for the release notes. I'll need to push something else.

A lot of work to be done here, but that has to be after the next release.
@robbietuk
Copy link
Collaborator

I agree with the change and it looks good. I dont have time to fully review it. Are there any additional tests that should be added?

@KrisThielemans
Copy link
Collaborator Author

Are there any additional tests that should be added?

I guess so. However, the old functions are now mostly implemented in terms of the new ones, so we have the tests for free. There will be a few untested, but we will find out once we apply this to TOF.

I wanted to have this PR on 5.2, such that we can tell people to use the new interface, and it will work for the next STIR version as well (while the old functions won't work for TOF). Maybe I didn't make the latter very clear in the release notes....

@KrisThielemans
Copy link
Collaborator Author

@danieldeidda this should be ready now, aside from some tiny edits to the release notes.

Copy link
Collaborator

@danieldeidda danieldeidda left a comment

Choose a reason for hiding this comment

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

looks good to me

src/include/stir/Bin.h Show resolved Hide resolved
src/include/stir/Bin.inl Show resolved Hide resolved
- fixes to text on SegmentIndices etc
- flag that we will require C++ 14 in the next major version
@KrisThielemans KrisThielemans merged commit ff1c20b into UCL:master Oct 18, 2023
5 checks passed
@KrisThielemans KrisThielemans deleted the ProjDataIndices branch October 18, 2023 21:21
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.

better indices for projection data
3 participants