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.
https://github.com/SyneRBI/SIRF/pull/635
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
@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 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 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 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 pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
Merged #635 into master.