[RFC] ENH: Always sync qforms, refactor error messaging #851
Conversation
This file contains 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
effigies
force-pushed
the
bold_qform_sync
branch
from
df30e54
to
79865a1
Compare
oesteban
reviewed
Nov 27, 2017
fmriprep/interfaces/images.py
Outdated
| @@ -370,47 +370,54 @@ def _run_interface(self, runtime): | |||
| # Check affine information is valid | |||
| valid_codes = (qform_code, sform_code) != (0, 0) | |||
|
|
|||
| # Matching affines | |||
| matching_affines = np.allclose(img.get_qform(), img.get_sform()) | |||
There was a problem hiding this comment.
If valid_qform is False this line will fail: img.get_qform() will raise a ValueError.
fmriprep/interfaces/images.py
Outdated
| <p class="elem-desc">Input file does not have a valid qform matrix.</p> | ||
| """ | ||
|
|
||
| if not (valid_qform and matching_affines): |
There was a problem hiding this comment.
like before, if qform is not valid then it doesn't make much sense to see if it matches the sform (or, better said, you can't even check).
There was a problem hiding this comment.
This is a short-circuit. We want to set the qform in the two cases: not valid_qform and not matching_affines.
There was a problem hiding this comment.
Yes, I just wanted to point out the fact that we need to set matching_affines in a different way for this switch to work as expected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #847, intended to clarify my opinions on this interface.
First, I think this is the easiest place to go ahead and sync qforms and sforms, unconditionally.
Second, I've refactored the warnings to follow this logic:
Just as some after-the-fact commentary on #847: I think the warnings for invalid qforms are weird, in that a qform_code of 0 is treated as intensifying the problem with invalid qforms, rather than mitigating it. It's bad and marked as bad. That seems like some software did a reasonable thing, even if it's now something we have to clean up after.
Relatedly, I think the only time that we need to say analyses "MAY BE INVALID" is when there is no usable qform or sform. A good sform but bad qform is a totally reasonable situation to be in, if it weren't for our choice of using ANTs. It's thus our responsibility to handle that case, and make clear what we've done, but we know from the headers what the correct orientation of the image is.
I'm okay with keeping code 1 as the default. I think that makes sense when we use the default affine. However, if we sync the sform, we should use whatever code it has.