Fix mrdata dimensions #1161
Fix mrdata dimensions #1161
Conversation
| @@ -962,8 +962,8 @@ def dimensions(self): | |||
| if self.number() < 1: | |||
| return numpy.zeros((MAX_ACQ_DIMENSIONS,), dtype=cpp_int_dtype()) | |||
| dim = numpy.ones((MAX_ACQ_DIMENSIONS,), dtype=cpp_int_dtype()) | |||
| hv = pygadgetron.cGT_getAcquisitionDataDimensions\ | |||
| (self.handle, dim.ctypes.data) | |||
| hv = try_calling(pygadgetron.cGT_getAcquisitionDataDimensions\ | |||
There was a problem hiding this comment.
try_calling has no return value (so the next line is no longer needed) - see src/common/Utilities.py
There was a problem hiding this comment.
to be sure, by the next line I mean line 967 (try_calling takes care about the handles returned by C interface functions)
| int counter_first_valid_acq = 1; | ||
| while(TO_BE_IGNORED(acq) && counter_first_valid_acq < this->number()) | ||
| { | ||
| get_acquisition(counter_first_valid_acq, acq); | ||
| counter_first_valid_acq++; | ||
| } | ||
|
|
||
| int ns = acq.number_of_samples(); | ||
| int nc = acq.active_channels(); | ||
|
|
||
| for(int i=1; i<na; ++i) | ||
| for(int i=counter_first_valid_acq; i<na; ++i) |
There was a problem hiding this comment.
same edits for get_trajectory_dimensions and get_kspace_dimensions?
There was a problem hiding this comment.
Actually, the situation is more complicated, I am afraid. If some acquisitions are to be ignored, dim[2] needs to be reduced accordingly in order for fill and as-array to work with correct dimensions, and all computations with acquisition data must do TO_BE_IGNORED checks, which makes it rather pointless to keep ignored acquisitions at all. And if we do not keep them, then TO_BE_IGNORED would only be needed when reading from file, and I believe we should issue warning message for every ignored acquisition to make the users aware that the data SIRF reads is not identical to the data in the file.
This all should be well thought through and discussed, so I suggest we postpone this PR and PR #1158 until after Release 3.4.
There was a problem hiding this comment.
And if we do not keep them, then
TO_BE_IGNOREDwould only be needed when reading from file, and I believe we should issue warning message for every ignored acquisition to make the users aware that the data SIRF reads is not identical to the data in the file.
Yes, I agree, but as mentioned in #1156 then gadgetron reconstructions seem not to work any more without the noise sample readouts, which was the reason for trying to drop the macro.
There was a problem hiding this comment.
It turns out that gadgetron_ismrmrd_client using Generic_Cartesian_Grappa.xml without conversion to unsigned shorts produces expected images.
Apparently, the pixel values of the reconstructed images are too small to be represented by non-zero unsigned shorts, which suggests Gadgetron scaling problems in the absence of noise calibration data. I will create a Gadgetron issue for this bug when we switch to using up-to-date Gadgetron.
Meanwhile I suggest adding second int/bool argument all to MRAcquisitionData::read, defaulting to false, that would allow the user to disable ignoring acquisitions, and the same argument to AcquisitionsVector constructor-from-file etc. I volunteer to do that and also fix dimensions bugs (in fact, it looks like ignoring acquisitions in MRAcquisitionData::read was merely my quick fix for those bugs, so a proper fix is on me).
@johannesmayer are you joining our tomorrow zoom?
|
taken care of by #1174, closing... |
Changes in this pull request
Checks dimensions of k-space only for acquisitions that are not ignored.
Catches runtime errors that are caused by C++ code in sirf.Gadgetron.AcquisitionData.dimension()` function
Testing performed
Used code on file which previously caused wrong dimensions. Yielded correct dimensions afterwards.
Related issues
Fixes #1160
Checklist before requesting a review
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following: