[SyneRBI/SIRF] Extends SIRF data algebra possible operands to numpy arrays (PR #1358)
Evgueni Ovtchinnikov
Changes in this pull request
Testing performed
Related issues
fixes #1357
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/1358
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
File Changes
(1 file)
- M src/xSTIR/pSTIR/STIR.py (50)
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.
clone() will make a copy, which fill will then change again. I assume using get_uniform_copy() will be a bit faster as it avoids reading the data. Or do we have an allocator/constructor that doesn't initialise the data at all?
—
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#1358)Actually, get_uniform_copy cannot be any faster as it calls clone to make a copy and then fills it with the specified value:
def get_uniform_copy(self, value=1.0):
'''Initialises an instance of DataContainer based on the template'''
y = self.clone() # SIRF uses `DiscretisedDensity.clone()` to make a copy of `self` data
y.fill(value)
return y
—
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 left a comment (SyneRBI/SIRF#1358)ah. :-)
—
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 left a comment (SyneRBI/SIRF#1358)unrelated HDF5 linking error
—
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 approved this pull request.
Before merging, can you add a line to CHANGES.md? ("Restored functionality for algebraic operations mixing STIR.ImageData and numpy arrays." or something like that.)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
paskino left a comment (SyneRBI/SIRF#1358)I'll test with MaGeZ as soon as I can.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino commented on this pull request.
> @@ -625,6 +625,56 @@ def as_array(self):
try_calling(pystir.cSTIR_getImageData(self.handle, array.ctypes.data))
return array
+ def dot(self, other):
+ '''
+ Returns the dot product of the container data with another container
+ data or numpy array viewed as vectors.
+ other: DataContainer
+ '''
+ if not (issubclass(type(other), type(self))):
+ other = self.clone().fill(other)
This sounds like a bad idea. It will double the memory usage.
—
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.
- c53d80b added to User Guide a subsection on SIRF/numpy data algebra peculiarities [ci skip]
- 9f2ecf8 updated CHANGES.md [ci skip]
—
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.
> @@ -625,6 +625,56 @@ def as_array(self):
try_calling(pystir.cSTIR_getImageData(self.handle, array.ctypes.data))
return array
+ def dot(self, other):
+ '''
+ Returns the dot product of the container data with another container
+ data or numpy array viewed as vectors.
+ other: DataContainer
+ '''
+ if not (issubclass(type(other), type(self))):
+ other = self.clone().fill(other)
yes, but it seems we have no alternative at the moment.
—
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 approved this pull request.
I'm fine with the doc. I'd still prefer to have a test before we merge though.
—
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.
- 80b3c07 added tests for #1358
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 57d9e15 attended to Codacy issues
—
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/xGadgetron/pGadgetron/Gadgetron.py:
> @@ -437,6 +437,56 @@ def get_ISMRMRD_info(self, par):
def get_info(self, par):
return self.get_ISMRMRD_info(par)
+ def dot(self, other):
+ '''
+ Returns the dot product of the container data with another container
+ data or numpy array viewed as vectors.
+ other: ImageData or numpy array or scalar
+ '''
+ if not (issubclass(type(other), type(self))):
+ other = self.clone().fill(other)
+ return super(ImageData, self).dot(other)
according to doc, since Python 3.0, you can write
⬇️ Suggested change- return super(ImageData, self).dot(other) + return super().dot(other)
which is a nicer, and in fact seems to say that we can write this only once somewhere, as opposed to for Gadgetron/STIR/Reg etc. No idea how to do that though.
—
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.
- 0477cc0 simpliied mixed data algebra
—
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 CHANGES.md:
> @@ -1,5 +1,10 @@ # ChangeLog +## v3.9.1 + +* Python interface + - Restored functionality for algebraic operations mixing STIR.ImageData and numpy arrays.⬇️ Suggested change
- - Restored functionality for algebraic operations mixing STIR.ImageData and numpy arrays. + - Restored functionality for algebraic operations mixing STIR.ImageData and numpy arrays. (Note that sirf objects need to be on the "left" of the operation.)
—
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.
- 87a8ddd Update CHANGES.md
—
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#1358)squash-merge on this.
—
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 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.