Error raised in AcquisitionSensitivityModel.[un]normalise methods applied to a read-only object

[evgueni-ovtchinnikov]

Changes in this pull request

Error raised in AcquisitionSensitivityModel.[un]normalise methods if applied to a read-only object.

Testing performed

Yes

Related issues

fixes #1298

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.

[KrisThielemans]

I'm a bit confused about the catching of a STIR error. As you saw, python just exited due to uncaught exception presumably. I'm not sure what try_calling does, but it looks like it didn't catch it.

Calling STIR functions could always get into stir.error, so I think it'd be safest to make sure we catch that somehow. Easy?

[evgueni-ovtchinnikov]

So far as I can see, methods undo and apply of BinNormalisation failed when applied to data that was supposed to be read-only but did not throw an exception, otherwise this exception would be caught by try in cSTIR_applyAcquisitionSensitivityModel and reported by try_calling (which just checks the execution status recorded in the DataHandle object returned by every C-interface function, in this case by cSTIR_applyAcquisitionSensitivityModel).

[paskino]

Build error:

In file included from /usr/include/dcmtk/oflog/config.h:31,
                 from /usr/include/dcmtk/oflog/logger.h:28,
                 from /usr/include/dcmtk/oflog/oflog.h:28,
                 from /usr/include/dcmtk/dcmdata/dctypes.h:27,
                 from /usr/include/dcmtk/dcmdata/dctk.h:28,
                 from /home/runner/work/SIRF/SIRF/build/sources/Gadgetron/gadgets/dicom/DicomFinishGadget.h:24,
                 from /home/runner/work/SIRF/SIRF/build/sources/Gadgetron/gadgets/dicom/DicomFinishGadget.cpp:4:
/usr/include/dcmtk/ofstd/ofstdinc.h:113:2: error: #error "Macro INCLUDE_CSTDIO not supported anymore. Include <cstdio> directly."
  113 | #error "Macro INCLUDE_CSTDIO not supported anymore. Include <cstdio> directly."
      |  ^~~~~
/usr/include/dcmtk/ofstd/ofstdinc.h:117:2: error: #error "Macro INCLUDE_CSTDLIB not supported anymore. Include <cstdlib> directly."
  117 | #error "Macro INCLUDE_CSTDLIB not supported anymore. Include <cstdlib> directly."
      |  ^~~~~
/usr/include/dcmtk/ofstd/ofstdinc.h:121:2: error: #error "Macro INCLUDE_CSTRING not supported anymore. Include <cstring> directly."
  121 | #error "Macro INCLUDE_CSTRING not supported anymore. Include <cstring> directly."
      |  ^~~~~

https://github.com/SyneRBI/SIRF/actions/runs/12870704245/job/35882381216?pr=1299#step:8:9365

[KrisThielemans]

The build error is due to gadgetron/gadgetron#1277. They haven't looked at my PR yet.

I've downgraded to GHA to use Ubuntu 22.04 in #1302, so merging master here would work-around this problem.

[KrisThielemans]

So far as I can see, methods undo and apply of BinNormalisation failed when applied to data that was supposed to be read-only but did not throw an exception, otherwise this exception would be caught by try in cSTIR_applyAcquisitionSensitivityModel and reported by try_calling (which just checks the execution status recorded in the DataHandle object returned by every C-interface function, in this case by cSTIR_applyAcquisitionSensitivityModel).

I don't know what's going on there, but as mentioned in #1298 (comment), STIR does throw an exception (see the error terminate called after throwing an instance of 'std::runtime_error'). So somehow SIRF didn't catch those.

[KrisThielemans]

The modifications to use the read_only attribute are good of course.