[SyneRBI/SIRF] STIR min/max-scale-factor in ScatterEstimation exposed. (PR #1271)
Evgueni Ovtchinnikov
Changes in this pull request
Added min/max_scale_factor methods to scatter estimation classes (C++ and Python).
Testing performed
Related issues
Fixes #1270.
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/1271
Commit Summary
- 52cf5e3 tackled #1270
File Changes
(3 files)
- M src/xSTIR/cSTIR/cstir_p.cpp (13)
- M src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h (10)
- M src/xSTIR/pSTIR/STIR.py (8)
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
@KrisThielemans requested changes on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
I prefer to have the Python and C++ names to be identical. As far as I can see, these currently are SIRF names in addition to the (already exposed?) STIR names? Maybe just cut this?
—
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 commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
STIR's set_min_scale_value cannot be called directly by user's code because of
class PETScatterEstimator : private stir::ScatterEstimation
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
I think using stir::ScatterEstimation::set_max_scale_value will work.
—
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 commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
/home/sirfuser/devel/buildVM/sources/SIRF/src/xSTIR/cSTIR/cstir_p.cpp: In function ‘void* sirf::cSTIR_setScatterEstimatorParameter(const DataHandle*, const char*, const DataHandle*)’:
/home/sirfuser/devel/buildVM/sources/SIRF/src/xSTIR/cSTIR/cstir_p.cpp:875:53: error: cannot call member function ‘void stir::ScatterEstimation::set_max_scale_value(float)’ without object
875 | stir::ScatterEstimation::set_max_scale_value(dataFromHandle<float>(hv))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
Either
void set_max_scale_factor(float v) { stir::ScatterEstimation::set_max_scale_value(v); } void set_min_scale_factor(float v) { stir::ScatterEstimation::set_min_scale_value(v); }
or
using stir::ScatterEstimation::set_max_scale_value; using stir::ScatterEstimation::set_min_scale_value;
the 2nd option is much more concise, but I prefer to use the first and add a doxygen string (which I don't think you can do with the 2nd option)
—
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 commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
The first way does not work - see the error message I reported above
The second way: where do I put using statements? I tried
class PETScatterEstimator : private stir::ScatterEstimation
{
using recon_type = stir::OSMAPOSLReconstruction<DiscretisedDensity<3,float>>;
using stir::ScatterEstimation::set_max_scale_value;
got
/home/sirfuser/devel/buildVM/sources/SIRF/src/xSTIR/cSTIR/cstir_p.cpp:880:32: error: ‘void stir::ScatterEstimation::set_max_scale_value(float)’ is inaccessible within this context
880 | obj.set_max_scale_value(dataFromHandle<float>(hv));
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
The 1st option will work. this is a modification of your own. The lines above need to be in the (public section) of the class def.
similarly for using. Needs to be after public.
—
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 commented on this pull request.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> + void set_max_scale_factor_value(float v)
+ {
+ set_max_scale_value(v);
+ }
+
+ void set_min_scale_factor_value(float v)
+ {
+ set_min_scale_value(v);
+ }
+
It turned out I was applying stir::ScatterEstimation:: prefix in a wrong place (in cstir_p.cpp) - all works now, 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.
- 004d968 accepted reviewers' suggestions
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
excellent.
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> @@ -947,6 +947,16 @@ The actual algorithm is described in
return this->get_reconstruction_method().get_num_subsets();
}
+ void set_max_scale_value(float v)
⬇️ Suggested change
- void set_max_scale_value(float v) + //! Set maximal scale factor value of the SSS algorithm to use + void set_max_scale_value(float v)
In src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:
> @@ -947,6 +947,16 @@ The actual algorithm is described in
return this->get_reconstruction_method().get_num_subsets();
}
+ void set_max_scale_value(float v)
+ {
+ stir::ScatterEstimation::set_max_scale_value(v);
+ }
+
+ void set_min_scale_value(float v)
⬇️ Suggested change
- void set_min_scale_value(float v) + //! Set minimal scale factor value of the SSS algorithm to use + void set_min_scale_value(float v)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans pushed 1 commit.
- cfbbb79 add doxygen [ci skip]
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans approved this pull request.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
Please add a line to CHANGES.md and merge (pull first)
—
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 2 commits.
- 535a8ab updated CHANGES.md to prepare for merging PR #1271
- 8a5b817 Merge branch 'scale-factor' of https://github.com/SyneRBI/SIRF into scale-factor
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
Merged #1271 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.