Add set get functions for OSEM reconstructor - ScatterEstimator

SyneRBI / SIRF

Main repository for the CCP SynerBI software

http://www.ccpsynerbi.ac.uk

Other

60 stars 29 forks source link

Add set get functions for OSEM reconstructor - ScatterEstimator #1223

Closed rmapree closed 11 months ago

rmapree commented 11 months ago

Adding get and set function for OSEM subiterations and subsets for ScatterEstimator

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

[x] I have performed a self-review of my code
[x] I have added docstrings/doxygen in line with the guidance in the developer guide
[x] I have implemented unit tests that cover any new or modified functionality
[x] 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:

[x] 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.

KrisThielemans commented 11 months ago

https://github.com/SyneRBI/SIRF/actions/runs/6973326347/job/18977119327?pr=1223#step:8:15010

/home/runner/work/SIRF/SIRF/src/xSTIR/cSTIR/include/sirf/STIR/stir_x.h:711:17: error: ‘class sirf::PETScatterEstimator’ has no member named ‘recon_sptr’
  711 |           this->recon_sptr->set_num_subiterations(arg);

Turns out that recon_sptr is a local variable in the constructor. Sorry.

stir::ScatterEstimation doesn't have a get_reconstruction_method_sptr yet sadly. I'll push an ugly fix. (Therefore, do a git pull before you make any further changes)

KrisThielemans commented 11 months ago

@rmapree there are 3 trailing white-spaces listed at https://app.codacy.com/gh/SyneRBI/SIRF/pullRequest?prid=13103645

Can you also add 2 lines showing how to set num_subsets at https://github.com/SyneRBI/SIRF/blob/008d1ca4ed91c6976f13aa262937f7f1cbe8bd80/examples/Python/PET/scatter_estimation.py#L97-L98, something like

  # Could set number of subsets used by the OSEM reconstruction inside the scatter estimation loop.
  # Here we will set it to 7 (which is in fact the default), which is appropriate for the mMR
    set.set_OSEM_num_subsets(7)

KrisThielemans commented 11 months ago

@rmapree I've added a test now. It all works fine! I'll also address the other items above. and then merge