Merged
Conversation
- The strides in the aggregate header are expected to already be in absolute rather than symbolic; as such, the former manipulation was inappropriate. - Only perform manual manipulation of axis strides relative to the hedaer of the first input image in the scenario where concatenation is adding a new axis; if concatenation is occurring across an existing axis, do not perform any manipulation.
Prior to 41c02e9, erroneous phase encoding information was being exported to applytopup: - The padded DWI series erroneously possessed different strides to the original DWI series. - The text files to be used by applytopup were being generated by a command that did not explicitly manipulate the strides of the image, whereas the input images provided to applytopup did involve that manipulation. In addition, it was discovered that applytopup was not performing any manipulation of the data if the phase encoding axis was indicated to be on the third spatial axis; this turns out to have been doing nothing as opposed to an erroneous correction, but given this observation, a check has nevertheless been added to issue a warning where this occurs.
Lestropie
added a commit
that referenced
this pull request
Feb 15, 2024
- Fix erroneous modification of strides upon header concatenation; this replicates the fix included in 41c02e9 as part of #2806, but is necessary for the suite of fixes here to work. Note that this affects mrcat in addition to multi-image format handling. - Fix setting of strides upon PNG read. - Expand testing of PNG handling. Evaluation of read and write functionality is performed separately, with the reference data fully visible and verifiable within the test data repository. Some previous tests bundled read and write testing together, which obscured the erroneous intermediate interpretation.
Member
Author
Conflicts: bin/dwifslpreproc core/header.cpp
Member
Author
|
Looks like one of the other parts of this PR, specifically fixing image strides in the circumstance where concatenation is performed along a new additional image axis, was also fixed in 1cc476d as part of #2713. So after bringing this up to date with current |
jdtournier
approved these changes
Jul 23, 2025
Lestropie
added a commit
that referenced
this pull request
Aug 26, 2025
dwifslpreproc & mrcat: Strides fix
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.
Traced issue back to here, which ties all the way back to #1541.
Was trying to diagnose an issue with improper correction of susceptibility distortions, and this is the first issue I came across (which unfortunately for me is likely not the only issue...).
applytopupwas doing absolutely nothing to the input data, meaning that theeddybrain mask was aligned with the distorted data, whereas the whole intention of usingapplytopupis to produce a processing mask that approximates where the brain will be at completion of processing. It turns out that this is becausedwifslpreprocwas tellingapplytopupthat the phase encoding directions were "k" and "k-", and presumably internallyapplytopupis deeming this unacceptable and either zeroing out that component of the phase encoding vectors or just code branching to a straight copy-input-to-output. Unfortunately it doesn't provide any indication of this at the terminal.The stride manipulation within
concatenate<vector<Header>>()seems to have been outright incorrect, so I don't think that it should cause any problems, but before merging I might make the same change ondevand run the additional PNG tests in #2713 just to make sure it doesn't have any unintended consequences (existing CI should cover anything else).