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