Conversation
Prioritise using slice timing data from DICOM AcquisitionTime field over CSA TimeAfterStart
|
I wonder if it's worth generating slice timing vectors from all plausible sources, and then checking for supra-threshold differences between them. A warning could be issued if there's a discrepancy greater than some threshold, and the source to prioritise in that instance could be set via a config file entry. Might then get some community feedback on whether certain sources are better than others, or receive exemplars of when one fails. |
Lestropie
force-pushed
the
slice_timing_priority
branch
from
3301d59 to
be9db44
Compare
Lestropie
force-pushed
the
dev
branch
2 times, most recently
from
70031c3 to
6bf4cec
Compare
Lestropie
force-pushed
the
slice_timing_priority
branch
from
be9db44 to
3301d59
Compare
Conflicts: core/file/dicom/mapper.cpp
Captures the full slice timing vector from any of the three known sources of such information, makes a decision on which of those sources to proceed with, and issues a warning to the user in the case of potential ambiguity. As suggested in #2427.
|
@jdtournier This needs a run against your private DICOM tests; particularly interested if anything spits out a different slice timing vector to what it had previously. Also @bjeurissen if you have any sample data where you recall that MRtrix was getting this wrong, would be useful to add it to that test suite. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // base-10 scaling | ||
| for (size_t n = 0; n < image.images_in_mosaic; ++n) { | ||
| slices_timing_float.push_back(0.001 * image.mosaic_slices_timing[n]); | ||
| slices_timing_csamosaic_float.push_back(0.001 * image.mosaic_slices_timing[n]); |
There was a problem hiding this comment.
warning: narrowing conversion from 'double' to 'value_type' (aka 'float') [bugprone-narrowing-conversions]
slices_timing_csamosaic_float.push_back(0.001 * image.mosaic_slices_timing[n]);
^| min_acquisition_time = std::min(min_acquisition_time, default_type(frames[n]->acquisition_time)); | ||
| for (size_t n = 0; n != dim[1]; ++n) | ||
| slices_timing_float.push_back(default_type(frames[n]->acquisition_time) - min_acquisition_time); | ||
| slices_timing_acqtime.push_back(default_type(frames[n]->acquisition_time) - min_acquisition_time); |
There was a problem hiding this comment.
warning: narrowing conversion from 'default_type' (aka 'double') to 'value_type' (aka 'float') [bugprone-narrowing-conversions]
slices_timing_acqtime.push_back(default_type(frames[n]->acquisition_time) - min_acquisition_time);
^| for (size_t n = 0; n != dim[1]; ++n) | ||
| min_time_after_start = std::min(min_time_after_start, frames[n]->time_after_start); | ||
| for (size_t n = 0; n != dim[1]; ++n) | ||
| slices_timing_csatimeafterstart.push_back(frames[n]->time_after_start - min_time_after_start); |
There was a problem hiding this comment.
warning: narrowing conversion from 'default_type' (aka 'double') to 'value_type' (aka 'float') [bugprone-narrowing-conversions]
slices_timing_csatimeafterstart.push_back(frames[n]->time_after_start - min_time_after_start);
^| (slices_timing_acqtime.empty() ? 0 : 1) + // | ||
| (slices_timing_csatimeafterstart.empty() ? 0 : 1); // | ||
|
|
||
| auto write_slice_timing_info = [&](const std::vector<float> &timing_vector, const std::string timing_string) { |
There was a problem hiding this comment.
warning: the const qualified parameter 'timing_string' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
| auto write_slice_timing_info = [&](const std::vector<float> &timing_vector, const std::string timing_string) { | |
| auto write_slice_timing_info = [&](const std::vector<float> &timing_vector, const std::string& timing_string) { |
2 participants
Reported by @bjeurissen.
Between two DWI series, one stored in mosiac format and one not, the current code produces a clearly erroneous slice timing vector in the latter case. The proposed change results in the use of DICOM field
AcquisitionTimein the latter case, which produces an identical result between the two datasets (ignoring floating-point precision, since slice timing for mosaics is reported and propagated as a string).Outstanding questions:
dcm2niixreports fractionally different slice timing for the non-mosaic dataset. I don't see any obvious source for this discrepancy. It's possible that it is deriving timings to a greater precision than what is reported in the DICOM headers, but I'm not sure.I don't recall whether the CSA
TimeAfterStartsource was prioritised overAcquisitionTimedue to behaviour in some other dataset, and therefore whether this change may result in erroneous behaviour elsewhere. @jdtournier Would your private DICOM test suite pick up such a change?