yeatmanlab / AFQ

Automated Fiber Quantification
74 stars 52 forks source link

mrTrix3 compatibility #11

Closed garikoitz closed 8 years ago

garikoitz commented 8 years ago

1.- In some cases changes are just for visualization (each parameter in a new line), but usually I add a new parameter (mrTrixVersion). 2.- I found a couple of bugs with mrTrix (not the correct parameters were being passed. 3.- I had to make changes in file/path names as well, it wasn't working for me, hopefully it would be ok, please check it. 4.- I had to create a copy of most of the vistasoft/mrtrix, maybe it will be easier to change them in Vistasoft and do a pull_request there, please let me know.

  1. mrTrix3 calls are quite different from mrTrix2, I code it in a way that the mrTrix2 calls were unaltered, but I didn't test them since I don't have mrTrix2 installed. 6.- WARNING: the results from mrTrix2 and mrTrix3 are not compatible.
jyeatman commented 8 years ago

Gari, thanks for the PR. @dstrodtman lets go through this together and test it on your data

jyeatman commented 8 years ago

I was just looking through this to follow the progress. Let me know when you think it is ready to be checked and merged

garikoitz commented 8 years ago

Jason, I think it is ok to check it again. The issues in the previous one have been addressed and now I have added the msmt options as well. You will have to change your preprocessing call in order to create concatenated files. There are some issues open to comment, if you want we can Skype right now.

garikoitz commented 8 years ago

Hi ,if you haven't started yet wait a little bit until the next commit, I am adding freesurfer to the 5ttgen options.

dstrodtman commented 8 years ago

Hi Gari,

The specific error that I'm getting from check_mrTrix_Version is the call [status, cmdout] = system('mrconvert -version'); cmdout =

mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: version GLIBCXX_3.4.19' not found (required by mrconvert) mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: versionGLIBCXX_3.4.18' not found (required by /home/dstrodtman/git/mrtrix3/release/bin/../lib/libmrtrix-0.3.15.so) mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: version `GLIBCXX_3.4.19' not found (required by /home/dstrodtman/git/mrtrix3/release/bin/../lib/libmrtrix-0.3.15.so)

It also appears that in AFQ_mrtrix_cmd it's defaulting to 5ttgen, which has broken the workaround that I had used with you code last Friday. The error being: Error: unrecognized arguments: -nthreads 0

usage: 5ttgen [-continue ] [-force] [-help] [-nocleanup] [-nthreads number] [-tempdir /path/to/tmp/] [-quiet | -verbose] {fsl,freesurfer} ...

This leads to a string of further errors, which I'm happy to share, but I'm hoping that your progress today will take care of these. Is it necessary to default to the 5ttgen? Do you know why the previous version of your code might have worked--has this option been added since then?

Thanks for all your work on this--let me know if there's a specific condition under which you'd like me to test this.

garikoitz commented 8 years ago

Thanks! Yes, the problem you have is the same one I had. Please see the other comments I did yesterday, with the Matlab call I posted this would be solved. In any case, as per Jason's suggestion, I already changed the check_mrTrix_Version and used only system('which '-s, so now it should work as before for you. I committed it yesterday.

About the 5ttgen: you need it if you have multishell data. The -nthreads 0 is working for me, that's strange. The problem I have now is with the /tmp folders. I am trying to troubleshoot it, but it seems something that they have to fix in their cpp code. The problem is that even if you specify to use your own /tmp folder, this parameter is not passed to the subsequent script calls, and those are the ones failing. This works fine in OSX, but in Linux in the cluster is not working. In the process I am testing several things, so maybe is better if you wait a little bit, I will comment it with the next commit. As a summary: I only have problems now with the 5ttgen script in Linux.

About the default: I added a param in AFQ_Create that makes it multishell = true, and if multishell is true, then you have to do 5ttgen.

On Tue, Jul 12, 2016 at 8:32 PM dstrodtman notifications@github.com wrote:

Hi Gari,

The specific error that I'm getting from check_mrTrix_Version is the call [status, cmdout] = system('mrconvert -version'); cmdout =

mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: version GLIBCXX_3.4.19' not found (required by mrconvert) mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: versionGLIBCXX_3.4.18' not found (required by /home/dstrodtman/git/mrtrix3/release/bin/../lib/ libmrtrix-0.3.15.so) mrconvert: /usr/local/MATLAB/R2014a/sys/os/glnxa64/libstdc++.so.6: version `GLIBCXX_3.4.19' not found (required by /home/dstrodtman/git/mrtrix3/release/bin/../lib/libmrtrix-0.3.15.so)

It also appears that in AFQ_mrtrix_cmd it's defaulting to 5ttgen, which has broken the workaround that I had used with you code last Friday. The error being: Error: unrecognized arguments: -nthreads 0

usage: 5ttgen [-continue ] [-force] [-help] [-nocleanup] [-nthreads number] [-tempdir /path/to/tmp/] [-quiet | -verbose] {fsl,freesurfer} ...

This leads to a string of further errors, which I'm happy to share, but I'm hoping that your progress today will take care of these. Is it necessary to default to the 5ttgen? Do you know why the previous version of your code might have worked--has this option been added since then?

Thanks for all your work on this--let me know if there's a specific condition under which you'd like me to test this.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232137751, or mute the thread https://github.com/notifications/unsubscribe/AEWQV2RE-rwLccmLBs6JFLRGKp8ij61Qks5qU93MgaJpZM4JGbEU .

dstrodtman commented 8 years ago

Adding the multishell as an optional varargin will make your code backwards compatible with everyone else's processing pipeline.

And I realized after my previous post that I've not reinstalled Freesurfer since I rebuilt my OS. I'll do that now. But given the code that you currently have written, I can bypass this by making multishell = 0 in AFQ_Create, given that I have single shell data? Testing now.

dstrodtman commented 8 years ago

Setting multishell = false, I get the following error.

Reference to non-existent field 'wmCsd'. Error in AFQ_Create (line 303) afq.files.mrtrix.csd{ii} = files.wmCsd;

garikoitz commented 8 years ago

@dstrodtman you can check AFQ_Create now if you want. Don't try with multishell yet please, I am changing AFQ_mrtrix_5ttgen right now, maybe I will change AFQ_mrtrixInit as well but as you said if multishell is 'false' it should be compatible.

jyeatman commented 8 years ago

It would be best to split these pull requests to separate branches. It makes it difficult for us to check anything if aspects of the code are still changing

Jason D. Yeatman, PhD Assistant Professor, Institute for Learning & Brain Sciences (I-LABS) Department of Speech & Hearing Sciences University of Washington http://brainandeducation.com

On Tue, Jul 12, 2016 at 4:17 PM, Garikoitz Lerma-Usabiaga < notifications@github.com> wrote:

@dstrodtman https://github.com/dstrodtman you can check AFQ_Create now if you want. Don't try with multishell yet please, I am changing AFQ_mrtrix_5ttgen right now, maybe I will change AFQ_mrtrixInit as well but as you said if multishell is 'false' it should be compatible.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232210420, or mute the thread https://github.com/notifications/unsubscribe/ABsYynTz8ac3yVFThmEAKba1nskyTJzKks5qVCB_gaJpZM4JGbEU .

garikoitz commented 8 years ago

Yes sorry, it is my very first pull request and it is too big and in a rush. I will stop it now, in linux is working fine, in osx is working fine, I am going to check that both multishell and not-multishell are working and I will leave it like that.

In the cluster is failing a mrtrix system command, it is not going to affect AFQ.

I will commit and write a comment soon.

On Wed, Jul 13, 2016 at 2:30 AM Jason D. Yeatman notifications@github.com wrote:

It would be best to split these pull requests to separate branches. It makes it difficult for us to check anything if aspects of the code are still changing

Jason D. Yeatman, PhD Assistant Professor, Institute for Learning & Brain Sciences (I-LABS) Department of Speech & Hearing Sciences University of Washington http://brainandeducation.com

On Tue, Jul 12, 2016 at 4:17 PM, Garikoitz Lerma-Usabiaga < notifications@github.com> wrote:

@dstrodtman https://github.com/dstrodtman you can check AFQ_Create now if you want. Don't try with multishell yet please, I am changing AFQ_mrtrix_5ttgen right now, maybe I will change AFQ_mrtrixInit as well but as you said if multishell is 'false' it should be compatible.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232210420, or mute the thread < https://github.com/notifications/unsubscribe/ABsYynTz8ac3yVFThmEAKba1nskyTJzKks5qVCB_gaJpZM4JGbEU

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232221759, or mute the thread https://github.com/notifications/unsubscribe/AEWQV5C9beJ2OV4ldRdvrtW63B4cfFhzks5qVDGVgaJpZM4JGbEU .

jyeatman commented 8 years ago

Okay sounds good. You can put this pull request on a separate branch so that you can keep developing while we go through and check all this

On Tuesday, July 12, 2016, Garikoitz Lerma-Usabiaga < notifications@github.com> wrote:

Yes sorry, it is my very first pull request and it is too big and in a rush. I will stop it now, in linux is working fine, in osx is working fine, I am going to check that both multishell and not-multishell are working and I will leave it like that.

In the cluster is failing a mrtrix system command, it is not going to affect AFQ.

I will commit and write a comment soon.

On Wed, Jul 13, 2016 at 2:30 AM Jason D. Yeatman <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

It would be best to split these pull requests to separate branches. It makes it difficult for us to check anything if aspects of the code are still changing

Jason D. Yeatman, PhD Assistant Professor, Institute for Learning & Brain Sciences (I-LABS) Department of Speech & Hearing Sciences University of Washington http://brainandeducation.com

On Tue, Jul 12, 2016 at 4:17 PM, Garikoitz Lerma-Usabiaga < notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

@dstrodtman https://github.com/dstrodtman you can check AFQ_Create now if you want. Don't try with multishell yet please, I am changing AFQ_mrtrix_5ttgen right now, maybe I will change AFQ_mrtrixInit as well but as you said if multishell is 'false' it should be compatible.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232210420, or mute the thread <

https://github.com/notifications/unsubscribe/ABsYynTz8ac3yVFThmEAKba1nskyTJzKks5qVCB_gaJpZM4JGbEU

.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232221759, or mute the thread < https://github.com/notifications/unsubscribe/AEWQV5C9beJ2OV4ldRdvrtW63B4cfFhzks5qVDGVgaJpZM4JGbEU

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-232222676, or mute the thread https://github.com/notifications/unsubscribe/ABsYymWavrSjFQDcISHrThGL3WKkGT4Hks5qVDL4gaJpZM4JGbEU .

Jason D. Yeatman, PhD Assistant Professor, Institute for Learning & Brain Sciences (I-LABS) Department of Speech & Hearing Sciences University of Washington http://brainandeducation.com

garikoitz commented 8 years ago

Thank you both for your patience. I have checked with several subjects from preprocessing until AFQ_Create and AFQ_Run, in OSX, in Linux and in Cluster Linux, with multishell = true and = false, and it is working fine for me. Multishell = false is the default.

There are notes in the code about the cluster issue. Basically it is related to how the /tmp folders are being used. I pass the -tempdir command now specifying which temp folder I want to use.

Please remember that there is some issues with the folders. Now I assume that it is creating in the proper place and with the proper prefix (at least for me), but as I don't know what is your setup I cannot check it.

mrTrix2 hasn't been checked.

Last question: Jason, do you usually use acpc-d T1w or the one from freesurfer without the AC-PC? It is going to be easier to plot in the surface and using the segmentations to use without the AC-PC right? If you use multishell with the 'freesurfer' option ('fsl' is the default), it will require aparc+aseg.mgz. We could rotate aparc+aseg.mgz to ac-pc as well with nearest neighbour (they are labels), but the results are not going to be very good. And then I don't know what the implications are for surfaces (do we need to register them as well?). What do you usually do in this regard? Right now I decided to work in FS space without ac-pc rotating and registering all DWI data to T1.mgz.

I won't touch the code anymore until you check it.

Thanks again and please let me know if you find any issue.

dstrodtman commented 8 years ago

Hi Gari, I haven't had time to go through the results with Jason, but it does appear that processing a single shell is now working properly on my system. Thank you! Hopefully Jason and I will have time to run over the results soon and I can confirm that everything is in order.

I tried to run multishell without reading your comments above and ran into that exact issue. I'm concerned about hard-coding a file that isn't generating during the AFQ process and hasn't previously been required to run the pipeline. If multishell processing requires Freesurfer and additional files, perhaps the best solution would be to have multishell == true trigger an initial check that Freesurfer is installed and that paths have been defined to these additional files. That way it will throw an error immediately if these conditions aren't met.

dstrodtman commented 8 years ago

I'm going to be with subjects for the next few hours. I'll have a little time to spend later this afternoon on this, but our building's power is going down for the weekend so likely won't be able to do much until Monday at the earliest. Thanks for all your work on this!

garikoitz commented 8 years ago

Yes, it was a quick and dirty solution for the /tmp problem I was having. I have read some other reports online with the same problem. We can use any folder name (~/AFQ_mrtrix_tmp). The thing is that ~/ should always be writeable and there always should be space on disk. Can you change this directly or should I change and commit it?

On Fri, Jul 15, 2016 at 9:20 PM dstrodtman notifications@github.com wrote:

I'm going to be with subjects for the next few hours. I'll have a little time to spend later this afternoon on this, but our building's power is going down for the weekend so likely won't be able to do much until Monday at the earliest. Thanks for all your work on this!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-233046603, or mute the thread https://github.com/notifications/unsubscribe-auth/AEWQVyBwkPfEByxOJPZxI7qW7An3XpcPks5qV91jgaJpZM4JGbEU .

dstrodtman commented 8 years ago

Gari, You'd mentioned that you've been testing this on a compute cluster. AFQ_mrtrixInit is presently being called serially within a for loop in AFQ_Create. This is causing a huge bottleneck in my pipeline--does your set-up automatically work around this and allow for parallel processing of subjects, or are you just distributing the computation for a single subject?

garikoitz commented 8 years ago

I launched every subject in parallel, I call a function based on bde_preprocdiffusion for every subject in parallel.

On Sat, Jul 23, 2016 at 1:06 AM dstrodtman notifications@github.com wrote:

Gari, You'd mentioned that you've been testing this on a compute cluster. AFQ_mrtrixInit is presently being called serially within a for loop in AFQ_Create. This is causing a huge bottleneck in my pipeline--does your set-up automatically work around this and allow for parallel processing of subjects, or are you just distributing the computation for a single subject?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/yeatmanlab/AFQ/pull/11#issuecomment-234677921, or mute the thread https://github.com/notifications/unsubscribe-auth/AEWQV8r61HQRY-2eozlnqgcMbUmOHuiOks5qYUzcgaJpZM4JGbEU .

jyeatman commented 8 years ago

I am reorganizing all this in another PR