[SyneRBI/SIRF] Change prefix from PETAcquisitionData to STIRPETAcquisitionData. (PR #1146)
Evgueni Ovtchinnikov
Changes in this pull request
STIR now handles SPECT in addition to PET, so we should use prefix STIR for STIR data containers.
This was already in place for images - this pull request does the same for acquisition data.
For the sake of backward compatibility, old nomenclature is preserved for the time being via typedef.
Testing performed
Related issues
Fixes #1126.
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
- 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:
- 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.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF/pull/1146
Commit Summary
- 2daf037 PETAcquisitionData -> STIRAcquisitionData, fixes #1126
File Changes
(8 files)
- M src/xSTIR/cSTIR/cstir.cpp (80)
- M src/xSTIR/cSTIR/cstir_p.cpp (20)
- M src/xSTIR/cSTIR/include/sirf/STIR/stir_data_containers.h (163)
- M src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h (110)
- M src/xSTIR/cSTIR/stir_data_containers.cpp (76)
- M src/xSTIR/cSTIR/stir_x.cpp (30)
- M src/xSTIR/cSTIR/tests/test1.cpp (26)
- M src/xSTIR/cSTIR/tests/test4.cpp (10)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
thanks! We will need to put this in version 4.0, as it breaks backwards compatibility.
@johannesmayer presumably this will affect #1007 ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
We will need to put this in version 4.0, as it breaks backwards compatibility.
Actually, this PR can be merged right away as we have
///
/// Backward compatibility - to be removed in SIRF 4
///
typedef STIRAcquisitionData PETAcquisitionData;
typedef STIRAcquisitionDataInFile PETAcquisitionDataInFile;
typedef STIRAcquisitionDataInMemory PETAcquisitionDataInMemory;
(stir_data_containers.h, lines 772-777).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
ah. perfect. Great idea!
can you put those typedefs between #ifdef SIRF_VERSION < 040000 or whatever it has to be? Also add a line to CHANGES.md. Feel free to merge then. thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 152ddcd backward compatibility typedefs only used if SIRF version < 4.0.0
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 02d3d5e trying to fix build error (no version.h)
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 0daf83b [ci skip] updated CHANGES.md
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
Merged #1146 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
I notice the problem you had with version.h. I guess this means that we don't have access to the generated file
Correct? I think this means we need to add a line after https://github.com/SyneRBI/SIRF/blob/cf384b18ef5f000f9509457e6cb928971660ae2f/CMakeLists.txt#L213
include_directories(${CMAKE_CURRENT_BINARY_DIR}/cmake/include
If you think that's correct, please create an issue for it
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.