[SyneRBI/SIRF] Proper in-place algebra (#635)
Evgueni Ovtchinnikov
In-place algebra (+=, -=, *= and /=) was temporarily implemented in SIRF via numpy in-place algebra and as_array/fill.
This PR replaces the temproary implementation with a proper one that eliminates unnecessary copying of data between SIRF data containers and numpy arrays.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF/pull/635
Commit Summary
- started working on in-place data containers algebra
- implemented in-place + and -
- corrected mistake in SIRF.DataContainer.__iadd__
- implemented in-place * and /
File Changes
- M examples/Python/MR/acquisition_model.py (22)
- M examples/Python/PET/acquisition_data.py (14)
- M src/common/SIRF.py (12)
- M src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp (16)
- M src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_data_containers.h (9)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Kris Thielemans
@KrisThielemans commented on this pull request.
some comments. But do we have any tests for this? (There are some tests in the demo, but that doesn't get run in ctest)
In examples/Python/PET/acquisition_data.py:
> - diff = new_acq_data - acq_data + diff = new_acq_data + diff -= acq_data
this update looks a non-intuitive way to write the code. why would we do this?
In src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp:
> @@ -378,11 +378,13 @@ complex_float_t a, complex_float_t b) int n = y.number(); ISMRMRD::Acquisition ax; ISMRMRD::Acquisition ay; + ISMRMRD::Acquisition acq;
updates here don't seem have anything to do with the title of this PR. Are these intentional?
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_data_containers.h:
> @@ -222,6 +222,7 @@ namespace sirf {
// abstract methods
virtual void empty() = 0;
+ virtual void take_over(MRAcquisitionData&) = 0;
not related to PR?
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In examples/Python/PET/acquisition_data.py:
> - diff = new_acq_data - acq_data + diff = new_acq_data + diff -= acq_data
just was checking -=
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp:
> @@ -378,11 +378,13 @@ complex_float_t a, complex_float_t b) int n = y.number(); ISMRMRD::Acquisition ax; ISMRMRD::Acquisition ay; + ISMRMRD::Acquisition acq;
all updates are necessary for in-place algebra the way I implemented it: e.g. now that we want to overwrite acquisition data, we need to check first whether the acquisition to be overwritten is not to be ignored
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
In src/xGadgetron/cGadgetron/include/sirf/Gadgetron/gadgetron_data_containers.h:
> @@ -222,6 +222,7 @@ namespace sirf {
// abstract methods
virtual void empty() = 0;
+ virtual void take_over(MRAcquisitionData&) = 0;
directly related - actually, the main difference from the previous approach is that now new data is created before the old one is dismissed, after which the object takes the ownership of the new data and discards the old one
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 2 commits.
- f0a3e75 moved in-place algebra tests from examples to tests
- 06b289d Merge branch 'in-place-algebra' of https://github.com/CCPPETMR/SIRF into in-place-algebra
—
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
Merged #635 into master.