[SyneRBI/SIRF] sirf.Reg.ImageData.asarray() fix (PR #1376)
Evgueni Ovtchinnikov
Changes in this pull request
Testing performed
Related issues
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/1376
Commit Summary
- d95c7bb updated files for release 3.9.0 [ci skip]
- 54505e7 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- dedf83c Merged branch 'master' of https://github.com/SyneRBI/SIRF
- a164114 added numpy array to possible SIRF image data algebra operands
- c53d80b added to User Guide a subsection on SIRF/numpy data algebra peculiarities [ci skip]
- 9f2ecf8 updated CHANGES.md [ci skip]
- 80b3c07 added tests for #1358
- 57d9e15 attended to Codacy issues
- 0477cc0 simpliied mixed data algebra
- 17867e4 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- f249547 resolved conflicts
- 5a1e82f Merge branch 'master' of https://github.com/SyneRBI/SIRF
- 6bfdf52 fixed mixing SIRF and numpy data algebra issue #1357
- 9b67e9e added algebra tests for ax/x, 2/x etc.
- a00488d Merge branch 'master' of https://github.com/SyneRBI/SIRF
- c3aa576 Merge branch 'master' into mda-priority-fix
- 856c7ee added the result type checks for mixed data algebra
- 0d8f93f [ci skip] updated User Guide description of data algebra and CHANGES.md
- 7ce286d fixed sirf.Reg.ImageData.asarray() bug
File Changes
(5 files)
- M CHANGES.md (2)
- M doc/UserGuide.md (2)
- M src/Registration/pReg/Reg.py (9)
- M src/common/SIRF.py (11)
- M src/common/Utilities.py (127)
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.
Need to say something about the Reg fix in CHANGES.md.
As the new functionality actually tested? As I don't think it's correct.
Also, in #1373 I suggested to duplicate tests with as_array() and asarray(), but maybe that isn't necessary.
Final comment, this is currently on top of #1373. Are we abandoning that one? The history here is rather awful, but a squash merge if the Reg fix and the algebra fix seems rather weird. Would then need some rebasing. Alternatively, we squash merge #1373 first, and then merge master here etc. No easy solutions... @casperdcl, we'll need your help I think.
> @@ -141,6 +146,9 @@ def __sub__(self, other):
'''
return self.subtract(other)
+ def __rsub__(self, other):
+ return -other + self
I believe this is incorrect, as y.__rsub__(x) == x-y, so shouldn't it be -self + other?
I have no idea about overloads, but both implementations seem to first multiply with -1 and then add, which is normally wasteful. I guess if we have a constructor that doesn't do any work, we could do something like
try ret = self.allocate() ret.asarray() = other.asarray() - self.asarray() return ret except someExceptionStuffHere: return -self + other
> @@ -171,6 +179,9 @@ def __truediv__(self, other):
'''
return self.divide(other)
+ def __rtruediv__(self, other):
+ return other*self.power(-1)
similar comments as for __rsub__
In src/Registration/pReg/Reg.py:
> @@ -567,7 +567,14 @@ def __array_interface__(self):
"""As per https://numpy.org/doc/stable/reference/arrays.interface.html"""
if not self.supports_array_view:
raise ContiguousError("please make an array-copy first with `asarray(copy=True)` or `as_array()`")
- return {'shape': self.shape, 'typestr': '<f4', 'version': 3,
+ shape = self.shape
+ dims = len(shape)
+ strides = ()
+ stride = 4
please add some comment on why this is the case
—
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.
- 608074e made corrections required by replacing as_array with asarray in data algebra tests
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 1e65fb5 [ci skip] explained strides setting in sirf.Reg.ImageData.__array_interface__
—
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.
> @@ -141,6 +146,9 @@ def __sub__(self, other):
'''
return self.subtract(other)
+ def __rsub__(self, other):
+ return -other + self
I believe this is incorrect, as
y.__rsub__(x) == x-y, so shouldn't it be-self + other?
apparently not, otherwise respective data container algebra tests would fail (also in CIL, from which this approach was adopted)
ret.asarray() = other.asarray() - self.asarray()
would not work for MR data, I am afraid
—
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/Registration/pReg/Reg.py:
> @@ -567,7 +567,14 @@ def __array_interface__(self):
"""As per https://numpy.org/doc/stable/reference/arrays.interface.html"""
if not self.supports_array_view:
raise ContiguousError("please make an array-copy first with `asarray(copy=True)` or `as_array()`")
- return {'shape': self.shape, 'typestr': '<f4', 'version': 3,
+ shape = self.shape
+ dims = len(shape)
+ strides = ()
+ stride = 4
added here and in CHANGES.md
—
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 left a comment (SyneRBI/SIRF#1376)@KrisThielemans To clean up the history, I suggest we merge #1373 before #1376.
—
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 4 commits.
- 6d1d3e5 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- a50a20c asarray -> as_array
- 07e4b6a moved t = y.norm() back to where it was on master
- ff2a60b Merge branch 'master' into reg-asarray-fix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 6bce975 trying to placate Codacy
—
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/Registration/pReg/Reg.py:
> + """NiftiImageData stores voxels values in a 3D Fortran-style array, + hence strides need to be arranged accordingly (default is C-style)."""
Codacy complains because of a string. I guess you intended this as a docstring, in which case it would have be moved up. However, I think it's fine to just have it as a comment (no string).
—
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.
- 0ab9500 Merge branch 'master' into reg-asarray-fix
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF#1376)ok, this is now a very small PR. Great! Once ready, please use squash-merge.
Question: is the output of the old as_array() and new asarray() (and presumably therefore new as_array(), as you're testing that) the same? (i.e. do they use the same order). I guess so, as you haven't touched Reg.ImageData.as_array(), which is currently
https://github.com/SyneRBI/SIRF/blob/24483dc77942a5ac683b8d55809a43c7b8862a7b/src/Registration/pReg/Reg.py#L372
We cannot change that, as otherwise, we'll get into trouble with the registration tools and geometry.
This does explain some choices that Richard made in the geometric info code (which don't work for STIR, but that's another PR)
—
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 left a comment (SyneRBI/SIRF#1376)yes, as_array returns the same data as asarray
@KrisThielemans please approve, I cannot merge until you approve
—
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/Registration/pReg/Reg.py:
> + """NiftiImageData stores voxels values in a 3D Fortran-style array, + hence strides need to be arranged accordingly (default is C-style)."""
- """NiftiImageData stores voxels values in a 3D Fortran-style array, - hence strides need to be arranged accordingly (default is C-style).""" + # NiftiImageData stores voxels values in a 3D Fortran-style array, + # hence strides need to be arranged accordingly (default is C-style).
—
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.
- 0c7707c Replace string with comment
—
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.
All fine, except that CHANGES.md was based on an old version, so you lost some lines.
In CHANGES.md:
> @@ -4,13 +4,10 @@ * SIRF/STIR - The implementation of the creation of `sirf.STIR.ImageData` from `sirf.STIR.AcquisitionData` has been revised to ensure compatibility of `ImageData` dimensions and voxel sizes with `AcquisitionData`. - - Missing `__del__` added to sirf.STIR.AcquisitionModel. +* SIR/Registration⬇️ Suggested change
-* SIR/Registration +* SIRF/Registration
However, was Reg.ImageData.__array_interface__ already in 3.9.0? If not, this comment should just say that we added it.
In CHANGES.md:
> @@ -4,13 +4,10 @@ * SIRF/STIR - The implementation of the creation of `sirf.STIR.ImageData` from `sirf.STIR.AcquisitionData` has been revised to ensure compatibility of `ImageData` dimensions and voxel sizes with `AcquisitionData`. - - Missing `__del__` added to sirf.STIR.AcquisitionModel.
seems this line shouldn't have been deleted. (Note the wrong - - though)
In CHANGES.md:
> - - Error raised if `AcquisitionModel.adjoint` ran when the model is not linear. -* SIRF - - `common/SIRF.py` adding adjoint operator
should not have been deleted
In CHANGES.md:
> @@ -30,8 +27,6 @@ - Error raised in `AcquisitionSensitivityModel.[un]normalise` methods applied to a read-only object. - SIRF interfaces (C++ and Python) for STIR Poisson noise generation utilities provided. - Python: - - allow in-place call of `ObjectiveFunction` `gradient` in Python. Added unit test for new functionality in `gradient` and for the `out` parameter.
should not have been deleted
—
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 3 commits.
- 109496f [ci skip] restored missing lines in CHANGES.md
- 4ebe9a9 Merge branch 'reg-asarray-fix' of https://github.com/SyneRBI/SIRF into reg-asarray-fix
- df9feae [ci skip] pulled and fixed CHANGES.md
—
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 CHANGES.md:
> - - Error raised if `AcquisitionModel.adjoint` ran when the model is not linear. -* SIRF - - `common/SIRF.py` adding adjoint operator
restored, rephrased
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
> @@ -4,13 +4,10 @@ * SIRF/STIR - The implementation of the creation of `sirf.STIR.ImageData` from `sirf.STIR.AcquisitionData` has been revised to ensure compatibility of `ImageData` dimensions and voxel sizes with `AcquisitionData`. - - Missing `__del__` added to sirf.STIR.AcquisitionModel. +* SIR/Registration
Yes, it was added 9 month ago, and 3.9.0 was released in Oct 2025.
—
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 CHANGES.md:
> @@ -4,13 +4,10 @@ * SIRF/STIR - The implementation of the creation of `sirf.STIR.ImageData` from `sirf.STIR.AcquisitionData` has been revised to ensure compatibility of `ImageData` dimensions and voxel sizes with `AcquisitionData`. - - Missing `__del__` added to sirf.STIR.AcquisitionModel.
restored
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
> @@ -30,8 +27,6 @@ - Error raised in `AcquisitionSensitivityModel.[un]normalise` methods applied to a read-only object. - SIRF interfaces (C++ and Python) for STIR Poisson noise generation utilities provided. - Python: - - allow in-place call of `ObjectiveFunction` `gradient` in Python. Added unit test for new functionality in `gradient` and for the `out` parameter.
restored
—
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 CHANGES.md:
> - Error raised if `AcquisitionModel.adjoint` ran when the model is not linear. -* SIRF - - `common/SIRF.py` adding adjoint operator - - addition of initial pytorch wrappers and examples, check `torch/README.md`
This (important!) line still disappeared!
—
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.
- eb7d1de [ci skip] restored the last (hopefully) couple of lines in CHANGES.md
—
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 CHANGES.md:
> - Error raised if `AcquisitionModel.adjoint` ran when the model is not linear. -* SIRF - - `common/SIRF.py` adding adjoint operator - - addition of initial pytorch wrappers and examples, check `torch/README.md`
restored (according to Files changed)
—
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.
- 5f4e227 Merge branch 'master' into reg-asarray-fix
—
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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.