New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing strides in scripts for input to FSL #506

Merged
merged 36 commits into from Apr 15, 2016

Conversation

Projects
None yet
4 participants
@thijsdhollander
Copy link
Member

thijsdhollander commented Apr 1, 2016

I kicked off with the first changes, but we might want to consistently do this for some other scripts as well (even when it might not matter per se).

thijsdhollander added some commits Apr 1, 2016

thijsdhollander added some commits Apr 4, 2016

dwipreproc: update some comments and a file name to remove references…
… to the supposed flipping that was previously happening, but is now assumed to not be needed (and has been removed in a previous commit. Note that the removal of this flipping is still to be tested, in a scenario where the rpe_all option is used.
@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 4, 2016

Ok, so 2 main commits so far; essentially: in dwipreproc every occurrence of -stride +1,+2,+3,+4 has been replaced with -stride -1,+2,+3,+4. There's strong evidence that this change is correct, by testing on a real dataset with macroscopically known motion. The flipping that this caused before to the output of topup, is hypothesised to no longer be present; so the flip that was manually done on the field map, has been removed. This remains to be tested using data where the rpe_all option can be applied (and this latter particular change also only applies for that option). Finally, in the other commit, I fixed some comments to no longer mention this flipping, and changed a file name to also not explicitly suggest that some flipping had to be or was applied.

thijsdhollander and others added some commits Apr 6, 2016

5ttgen fsl strides: change is not breaking anything, results still ma…
…tch anatomy, but subtle differences along the extent of the (bet derived) brain mask
@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 7, 2016

So, investigated the potential impact on 5ttgen fsl: changing +1,+2,+3,+4 to -1,+2,+3,+4 does not affect the actual segmentation, and the results still match the anatomy in exactly the same manner as before. What does appear to be affected is the outline of the (bet generated) mask. Note 5ttgen fsl by default uses a pipeline consisting of calls to bet, fast and first; so worst case there might be a mix of conventions causing all sorts of unexpected interactions for the pipeline as a whole. Going by the assumption that the need for the stride change seems to be more likely than the previous version, I made this change in commit c02ab52 . But consider this still under investigation... (any further evidence welcome).

Here's the output before and after the change. Just showing the CSF tissue type, since that one shows the subtle differences along the brain mask:

Before:
before

After:
after

Note again that all segmentations inside the brain mask were unaffected by the change... the only difference really seems to be just the brain mask outline. You might want to open the above images in separate tabs or something, and flick back and forth to observe the small differences.

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 7, 2016

If those strides were going to affect anything in this script, I would expect it to be FIRST, and/or the standard_space_roi step (provides initial masking prior to BET). Might be worth looking more closely at those two - the latter is probably having a concomitant effect on the BET outline.

Having said that, it's entirely possible that FSL isn't handling the expected axis directions equivalently between all commands, in which case whether we should provide -stride +1,+2,+3 or -stride -1,+2,+3 will be command-dependent. I'd rather hear back from FMRIB as to what they can discover rather than doing a blanket change (even if it more-or-less doesn't affect the result).

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 7, 2016

Have run dwiprep -rpe_all on this branch, and am confident that the field map image aligns with the input images with the mrtransform axis flip omitted, as was implemented in 45defc1.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 7, 2016

Agreed that we still need to check with "the source" on that front. I'd be surprised if it were a mix of standards/spaces (but, admittedly, it's only an assumption); that would've (potentially) led to issues for FSL users as well, if they used some of those commands in conjunction. Even if the change is justified as-is, I'd still explicitly add the stride option for every separate input. Those changes should then probably also be restricted to the fsl.py script (at the moment, the most important one sits in the 5ttgen script itself, which is a bit weird, since at that point it's not yet specified that the fsl implementation/algorithm will be used).
Just based on output, it's a bit too hard to tell which one (or none, or a mix) would be the correct one. Some details seem to be slightly in favour of the change, but it's too subtle to be true evidence I'd say. Here's one such detail that seems to suggest it's super-slightly better after the change:

before_detail after_detail

The hot scale is the T1 image; the grey scale overlay is the CSF segmentation. Note the very small change in the middle of the image... It's something, but definitely not enough to be convincing.

Good to hear the flip-issue makes sense as well. This also seemed to make more sense from the point of view that "native" FSL users never reported issues with chaining those 2 commands together...

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 7, 2016

I'd be surprised if it were a mix of standards/spaces (but, admittedly, it's only an assumption); that would've (potentially) led to issues for FSL users as well, if they used some of those commands in conjunction.

I think I'll be surprised no matter what the answer is :-/ It would be weird if their own documentation says they conform to RAS, but the entire code based has been built around LAS for over a decade. Heck, our NIfTI export could still be bugged.

Those changes should then probably also be restricted to the fsl.py script (at the moment, the most important one sits in the 5ttgen script itself, which is a bit weird, since at that point it's not yet specified that the fsl implementation/algorithm will be used).

This can be fixed: just write the input image to e.g. input.mif without modifying any strides, then each individual algorithm is responsible for modification from there. Was probably just to save an mrconvert call; and the freesurfer algorithm doesn't care particularly.

Just based on output, it's a bit too hard to tell which one (or none, or a mix) would be the correct one.

One I'm more interested to see is whether the good-ol' run_first_all silent failure is more or less frequent with this change. Not sure if I have data to test that. (FIRST actually runs its own internal coordinate system... but I don't think that's an additional confound here).

@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 8, 2016

Have run dwiprep -rpe_all on this branch, and am confident that the field map image aligns with the input images with the mrtransform axis flip omitted, as was implemented in 45defc1.

Just recalled that in the version history, FSL 5.0.8 made a change to the header output of topup. I suspect they fixed that image having an identity transform, but it may also be worth testing this specific part against different FSL versions.

@jdtournier

This comment has been minimized.

Copy link
Member

jdtournier commented Apr 8, 2016

Right, after consultation with Mark Jenkinson and the FSL team, it seems the correct fix is actually to modify the bvecs handling - x-component needs to be flipped when transform determinant is positive. Regardless, I think it's probably still a good idea to make this change, the images will then be pretty much exactly as FSL has historically expected - less likely to introduce issues. But this would be entirely optional - as long as we fix the bvecs handling...

Note that this will definitely mess up people's existing data... If anyone tries to import RAS NIfTI data with bvecs/bvals that worked fine in MRtrix before, they won't work once we fix the bvecs handling. We'll have to make some serious announcements about this.

@bjeurissen

This comment has been minimized.

Copy link
Member

bjeurissen commented Apr 8, 2016

@thijsdhollander , those subtle unexpected differences you see when switching between RAS and LAS, could they be due to the fact that some of these algorithms are not entirely deterministic?

@jdtournier

This comment has been minimized.

Copy link
Member

jdtournier commented Apr 8, 2016

I'm thinking it might be worth merging this branch now, since the changes are beneficial and don't interfere with anything else. We can start a fresh pull request for the bvec handling - and then worry about the implications of that change and how best to handle the fallout...

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 10, 2016

@bjeurissen : yes, that's what I expected at some point as well. But I double checked it by rerunning (after the change) a second time, the outcome being exactly the same. So at least for (the combination of) all steps and FSL programs involved in 5ttgen, the full process appears to be entirely deterministic.

In the case of dwipreproc, I noticed the change didn't only affect the corrected gradient table, but also the specific (image) outcome for all diffusion weighted images (but not the b=0 images). I reckon this is because eddy uses the gradient directions to know which DWIs can be expected to appear more similar. If these gradients would be reoriented throughout the algorithm for this purpose, then it becomes really crucial to provide them correctly at the input.

I agree with @jdtournier about merging these changes already: they now do result in the correct output (images as well as gradients). The bvec/bval issue remains crucial (and should of course be followed up in a separate pull request), but these changes at least already avoid the modified scripts being afffected by that specific issue.

Apart from dwipreproc and 5ttgen, there's some other scripts that interact with FSL too; we might just change the strides in those as well, so as to conform with FSL's historical expectations.

@bjeurissen

This comment has been minimized.

Copy link
Member

bjeurissen commented Apr 10, 2016

I am also in favour of merging

@jdtournier

This comment has been minimized.

Copy link
Member

jdtournier commented Apr 10, 2016

Right, let's merge. Before we do though, there is the slightly thorny issue of how we handle this publicly. We'll have to come clean about the issue and the fact that it was caused by our incorrect interpretation of the bvecs. We'll need to provide some recommendations for users to avoid further problems, and under what circumstances they might need to rerun their processing.

I'm actually not too worried about dwipreproc, or any of the scripts for that matter. Those users who care about getting the reorientation right can simply rerun their scripts and move on. I'm far more concerned about what'll happen when we fix the bvecs handling. That'll mean a decent proportion of people's data that worked fine in MRtrix before won't work properly afterwards. Thankfully it should be trivial to detect when they're wrong, so it's not as bad as the SH basis change for instance. But we'll probably need to provide instructions for how to verify the bvecs, and how to correct them if needed...

Also, given that the issues are so closely related, it might be worth fixing the bvec handling now as part of the same merge, and make a single announcement about the problem. I'll try to work on that tonight - although I'll probably need some help with testing (I guess it would be a simple matter of running dtifit and dwi2tensor and verifying the PPDs in FSLView and MRView respectively). Also, any thoughts on how to handle the announcement would be welcome...

bvecs: fix import/export
As discussed in #506. While the bvecs format is relative to the image axes,
it also assumes that the images are stored using a LHS coordinate
system, as per the old Analyze format. In other words, for an image
stored in NIfTI-standard RAS, the bvecs are interpreted as if the image
was stored in LAS. Another way to put it (although much more ambiguous)
is that when interpreting bvecs, the image should be assumed
radiological even if it is actually stored neurological.
@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 14, 2016

So while I'm on the move, I just remembered there's still other scripts that interface with FSL as well. If we're going to consistently recommend the -1,... approach for interfacing with FSL, it'd probably be a good idea to update those as well, while we're at it?
dwipreproc is covered, as well as 5ttgen (but I always welcome double checking); one other one I can think of off the top of my head is dwibiascorrect?
Also, at the moment, 5ttgen is changed overall to -1,...; even though there's separate FSL and Freesurfer options for that one. In this case, the change doesn't / shouldn't affect the Freesurfer version. But for, e.g., dwibiascorrect, there's also the ANTS version; where I'm (just here and now, without testing) not fully sure whether that one can live with a -1,... change, without introducing a flip in the output bias field (I'm actually thinking this is what will specifically happen for the ANTS version; that one should probably still be fed +1,... NIfTI's).
Any other scripts on the TODO list?

Further tweaks to scripts for FSL bvecs fix
- Added '-stride -1,+2,+3' to a couple of locations where it wasn't used previously, and perhaps should be.
- 5ttgen and dwibiascorrect will only convert to LAS NIfTI if FSL commands are being used.
- Avoid unnecessary .nii usage: Any temporary images wholly under MRtrix control will use .mif.
@Lestropie

This comment has been minimized.

Copy link
Member

Lestropie commented Apr 14, 2016

Just went through and changed what seemed appropriate. Also changed 5ttgen and dwibiascorrect to only map to LAS if FSL is being used. Yet to test everything through though.

@thijsdhollander

This comment has been minimized.

Copy link
Member

thijsdhollander commented Apr 14, 2016

Great 👍 , thanks for that! As to testing: I think we should be safe enough if, for any script that was changed, a simple/common use case is run before and after the script change. In principle, there shouldn't be any gross changes to the output (but minor ones, on the other hand, might be expected). Sudden obvious flips are of course a major warning; in which case we should panic and run around in disarray before we proceed looking into the matter. :-)

Lestropie and others added some commits Apr 14, 2016

dwipreproc: Check header transform of topup output
In the original script, the header of the input image was imported into the field map output image of topup; this is because topup was erroneously writing that image with an identity transform.
Now, the transform of this image will be explicitly checked; and if it's not identical to that of the input image to topup, the user will be presented with a warning, and the script will perform the transform replacement.
Manual merge of master branch into fix_strides_for_FSL
Required manual merge of dwipreproc
Merge pull request #541 from MRtrix3/build_fix_icons
build: fix rebuild of icons for GUI apps
user doc: update image formats page
- add documentation for supported formats
- describe transform
- describe strides
user docs: sort scripts when generating list
This is to avoid needlessly updating the list when re-running the doc
generation, when the only thing that's changed in the order of the
scripts in the list...
Merge pull request #549 from MRtrix3/mrview_odf_set_type_on_open
ODF tool: fix assert failure when loading SH ODF with non-SH ODF in current display

@jdtournier jdtournier merged commit 3f00557 into master Apr 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jdtournier jdtournier deleted the fix_strides_for_FSL branch Apr 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment