mrconvert: fix silent DirectIO axis misbinding in copy_permute(); dwifslpreproc: fix missing f-string in field-map crop - #3432
Open
Rad-dude wants to merge 3 commits into
Conversation
Image<T>::create() gained a new std::optional<DirectIO> direct_io parameter (PR MRtrix3#3315, inserted between template_header and add_to_command_history). copy_permute()'s call site was never updated for the new signature and still passes 3 positional arguments: Image<T>::create(output_filepath, header_out, add_to_command_history) add_to_command_history (a bool, almost always true) silently binds to the new 3rd parameter (direct_io) instead of the 4th, because bool implicitly promotes to int, which DirectIO's `DirectIO(int axis)` constructor accepts. `true` becomes `1`, so every mrconvert output image is created with an unintended DirectIO(axis=1) request -- i.e. "make axis 1 the fastest-varying axis in memory" (Stride::contiguous_along_axis(1, header)). This silently reorders the in-RAM/on-disk voxel layout without a corresponding update to the exported gradient table: -export_grad_fsl (DWI::save_bvecs_bvals(), gradient.cpp) computes bvecs assuming the image's own axis order via File::NIfTI::adjust_transform(), which correctly reacts to the now-corrupted Stride::get(header) -- so the export logic itself is not at fault, it's given bad input. The result is an image/gradient-table mismatch on every NIfTI+FSL bvec export, independent of whether -strides is explicitly requested. Minimal repro (no eddy or -strides needed): mrconvert dwi.mif out.nii -export_grad_fsl out.bvec out.bval mrconvert out.nii rt.mif -fslgrad out.bvec out.bval mrinfo dwi.mif -export_grad_mrtrix orig.b mrinfo rt.mif -export_grad_mrtrix rt.b diff orig.b rt.b # non-identical before this fix; identical after The real add_to_command_history value was masked because it happened to fall back to its own default (true) either way, which is why this was invisible from a command-history perspective and only surfaced as silent gradient-orientation corruption in downstream DWI processing (e.g. dwifslpreproc's eddy round-trip, or any other -export_grad_fsl use). Fix: pass std::nullopt explicitly for direct_io so add_to_command_history correctly binds to its intended 4th parameter. Verified against KUL_NIS's KUL_clinical_fmridti.sh clinical DTI pipeline (dwifslpreproc -> dwibiascorrect -> dwi2fod -> tckgen): DEC map orientation and dwigradcheck-verified gradient/image consistency restored end-to-end.
Round-trips the shared dwi.mif test asset through NIfTI + FSL bvecs/bvals and checks the re-imported gradient table against the original via testing_diff_matrix. Catches the DirectIO argument misbinding fixed in the previous commit: fails with a clear per-element mismatch against the pre-fix binary (verified directly by reverting the fix, rebuilding, and confirming this test fails with exactly the row/column mismatch expected from the axis-1/axis-0 swap), and passes against the fix. Uses dwi.mif since it's already a shared asset referenced by several other binary tests (amp2response, amp2sh, dwi2tensor, dwiextract) -- no new test_data additions needed.
The mrconvert call that crops the eddy-derived field map for eddyqc
output was a plain string literal instead of an f-string:
run.command('mrconvert {field_map_image} field_map_unpad.nii {dwi_post_eddy_crop_option}')
while both of its sibling calls immediately before and after it
(cropping eddy_mask.nii and the post-eddy DWI series, same pattern)
correctly use f-strings. {field_map_image} and
{dwi_post_eddy_crop_option} were therefore passed to mrconvert as
literal, uninterpolated text rather than the actual image path and
crop option, so mrconvert received 3 positional arguments instead of
2 and failed immediately:
mrconvert: [ERROR] Expected exactly 2 arguments (3 supplied)
mrconvert: [ERROR] Usage: mrconvert input output
mrconvert: [ERROR] Yours: mrconvert {field_map_image} field_map_unpad.nii {dwi_post_eddy_crop_option}
Only reached when topup produces a field_map image and -eddyqc_all is
requested -- an -eddyqc_all + real topup field map run is enough to
hit this on any dataset/vendor, independent of the -rpe_* mode or scan
platform. Confirmed on origin/dev HEAD (fetched live, unfixed there
too) -- not something introduced on this branch or specific to this
checkout.
Fix: add the missing f prefix so both placeholders interpolate
correctly, matching the two neighbouring calls.
Reported via a real -eddyqc_all + -align_seepi -rpe_header run
(dwifslpreproc on Siemens multi-shell DWI data) that failed after
~60 minutes of eddy correction with exactly this error.
Rad-dude
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent fixes:
Fixes #<3431>
mrconvert: silent DirectIO axis misbinding in
copy_permute().Image<T>::create()gained a newstd::optional<DirectIO> direct_ioparameter (Fix deprecated std::shared_ptr<>::unique() on image writeback #3315), inserted between
template_headerandadd_to_command_history.copy_permute()'s call site was never updatedfor the new signature, so
add_to_command_history(a bool, almostalways
true) silently binds to the new 3rd parameter (direct_io)instead of the 4th —
truepromotes to1, so every mrconvert outputimage silently gets
DirectIO(axis=1). This reorders the on-disk voxellayout without a corresponding update to the exported gradient table,
producing an image/gradient-table mismatch on every NIfTI+FSL bvec
export via
-export_grad_fsl, independent of whether-stridesisexplicitly requested. Fix: pass
std::nulloptexplicitly fordirect_iosoadd_to_command_historybinds to its intended parameter.Includes a regression test.
dwifslpreproc: missing f-string prefix in field-map crop command.
The
mrconvertcall cropping the eddy-derived field map for-eddyqc_alloutput was a plain string literal instead of an f-string,so
{field_map_image}and{dwi_post_eddy_crop_option}were passed tomrconvertas literal uninterpolated text rather than the actual imagepath and crop option --
mrconvertthen received 3 positional argumentsinstead of 2 and failed immediately ("Expected exactly 2 arguments (3
supplied)"). Reproduces on any
-eddyqc_allrun where topup produces afield map, independent of vendor/scan platform. One-character fix (add
the missing
f), matching the pattern already used by the two identicalsibling
mrconvertcalls immediately before and after it.