WIP: Add Freesurfer 7 #67
Conversation
ENH: bump to FS 7.1.1
|
@PeerHerholz and @AndysWorth this resolves #57 with the potential exception of the replacement recon-all script. I'd be happy to get your feedback on it. |
|
Ahoi hoi @Shotgunosine, thx for the fantastic work and effort. The same to you @AndysWorth. |
|
Yeah, That's what I'm thinking. |
|
Awesome! @AndysWorth, do you want to have a look as well? After that or otherwise, we could go ahead and merge!? |
|
Sorry for the slow response. This looks good to me. I just noticed that I got a response on the Freesurfer mailing list from Doug Greve:
Thanks for your strong work @Shotgunosine! |
|
Cool, thanks @AndysWorth! @Shotgunosine should we go ahead and merge? WDYT re the |
|
We should add a sed command to the docker command, probably the best place
would be in neurodocker. @AndysWorth, do you think you could open an issue
there and submit a PR?
…On Fri, Nov 13, 2020 at 8:42 AM Peer Herholz ***@***.***> wrote:
Cool, thanks @AndysWorth <https://github.com/AndysWorth>! @Shotgunosine
<https://github.com/Shotgunosine> should we go ahead and merge? WDYT re
the recon-all script adaptation above?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABILLGBJ27Y7SRH3TGMV4MTSPUZU7ANCNFSM4TI2FGUA>
.
|
|
Will do! |
|
Hi folks, just checking: we can merge this, right? @Shotgunosine @AndysWorth |
|
Sorry, I was looking forward to making that contribution but it seems that I'm slowing this down. It looks like I won't have time until I'm off work for the holidays. @Shotgunosine if you want to go ahead with your suggestion in the meanwhile, that would be great. I know it's a small change but I don't feel confident to just do it without taking the time to make sure I fully understand what's going on. |
|
I might not be able to get to it until around the same time period. Maybe @llevitis would be able to take a look |
|
I've installed the fix here at Flywheel -- once I have had a chance to run it a few times I'll submit it to neurodocker. |
|
@AndysWorth have you had a chance to send the fix over to neurodocker? |
|
@Shotgunosine Sorry, no 😞 . There are two places that need patching in recon-all and the fixes have worked well at Flywheel so it's about time! I'll work on making the time. Thanks for the reminder. |
|
@AndysWorth, sounds good. It'd be great if you're able to make the changes this week, I've got a bunch of sessions I need to run and I'd love to be able to use this. |
|
Okay @Shotgunosine, I will! |
|
Hey @Shotgunosine , I (finally) managed to patch Freesurfer 7.7.1 in neurodocker. See neurodocker PR#384. I did this exact same change to the official Freesurfer docker image version of 7.1.1 and it has been run successfully many times on various Flywheel instances. I have not tested this new neurodocker version, which is for a slightly different version of centos (6 vs. 7). Is this PR sufficient for you to give it a try? All it should do is prevent crashing. |
|
@Shotgunosine I've created a repository recon-all-v7.1.1-parallel-patch. Right now it just has the patch file that can be installed with |
|
@AndysWorth, can you update the neurodocker PR to pull that repo run the bash script? |
|
@Shotgunosine I'm not familiar enough with neurodocker. Where would I do that? |
|
@Shotgunosine It sounds like they don't want the patch in neurodocker so the following should go somewhere around here? |
|
I’ll see if I can use the script you wrote to get a command into
neurodocker that’ll make them happy.
…On Thu, Apr 1, 2021 at 4:57 PM Andrew J. Worth ***@***.***> wrote:
@Shotgunosine <https://github.com/Shotgunosine> It sounds like they don't
want the patch in neurodocker so the following should go somewhere around
here
<https://github.com/Shotgunosine/freesurfer/blob/fs_7/generate_freesurfer_images.sh#L96>
?
curl -LJO https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch/tarball/master
tar zxf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe.tar.gz
cd AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
patch $FREESURFER_HOME/bin/recon-all parallel.patch
patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh quantifyThalamicNuclei.patch
cd ..
rm -rf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABILLGC46EKVCXGLUICFAPTTGTM4FANCNFSM4TI2FGUA>
.
|
|
With the new clarifications can you take care of it @AndysWorth?
…On Thu, Apr 1, 2021 at 5:15 PM Dylan Nielson ***@***.***> wrote:
I’ll see if I can use the script you wrote to get a command into
neurodocker that’ll make them happy.
On Thu, Apr 1, 2021 at 4:57 PM Andrew J. Worth ***@***.***>
wrote:
> @Shotgunosine <https://github.com/Shotgunosine> It sounds like they
> don't want the patch in neurodocker so the following should go somewhere
> around here
> <https://github.com/Shotgunosine/freesurfer/blob/fs_7/generate_freesurfer_images.sh#L96>
> ?
>
> curl -LJO https://github.com/AndysWorth/recon-all-v7.1.1-parallel-patch/tarball/master
> tar zxf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe.tar.gz
> cd AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
> patch $FREESURFER_HOME/bin/recon-all parallel.patch
> patch $FREESURFER_HOME/bin/quantifyThalamicNuclei.sh quantifyThalamicNuclei.patch
> cd ..
> rm -rf AndysWorth-recon-all-v7.1.1-parallel-patch-e5ff2fe
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#67 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABILLGC46EKVCXGLUICFAPTTGTM4FANCNFSM4TI2FGUA>
> .
>
|
|
will make a dummy change to retrigger circle ci |
|
getting somse validator error: will bump its version |
totally missed that you had done that and completely refactored the bash script: you've been busy |
|
I had to bump the node version to get one that was available for ubuntu-jammy, so I went ahead and bumped the bids validator version to the latest. I had only bumped node to 16.x and the validator says it needs at least 18.x so I've bumped node to 18.x and we'll try again. |
|
Bah, looks like circleci webhooks are broken at the moment. Is there anyone who can trigger a manual build? |
just did: let's see how this one goes |
|
see if we can create specific dependencies between the steps of the circle ci so that test for fs6 starts when the build for fs6 is done. Currently the tests can only start when both builds are done.
|
Yep, found the docs on matrix and they explain how to set it up. The circleci graph makes it looks like the dependencies are correct now. |
Dockerfile
Outdated
There was a problem hiding this comment.
shouldn't we just delete the Dockerfile?
nice!!! thanks for looking into this |
|
@Remi-Gau, the tests are failing because they can't find anything at /home/circleci/data/ds114_test1_freesurfer_precomp_v6.0.0/00_group*. That part of the circleconfig predates this PR. Do you know what's going on with that? |
|
get the same bug locally: # get download script
wget https://raw.githubusercontent.com/bids-apps/maintenance-tools/main/utils/get_data_from_osf.sh
# get gata into home dir from https://osf.io/9q7dv/
bash get_data_from_osf.sh ds114_test1_freesurfer_precomp_v6.0.0
# create target dir
mkdir -p ${HOME}/outputs1 ${HOME}/outputs2
# try to copy
sudo mv ${HOME}/data/ds114_test1_freesurfer_precomp_v6.0.0/00_group* ${HOME}/outputs1/no group folder $ ls ${HOME}/data/ds114_test1_freesurfer_precomp_v6.0.0/
fsaverage lh.EC_average rh.EC_average sub-01 sub-02let me dig into the history of the repo to see what was happening in the good old days |
|
here is the circle ci "step" that was used before I started working in the circle-ci: more readable docker run -ti --rm --read-only \
-v ~/license/license.txt:/license.txt \
--tmpfs /tmp --tmpfs /var/tmp \
-v ${HOME}/data/ds114_test1:/bids_dataset \
-v ${HOME}/data/ds114_test1_freesurfer_precomp_v6.0.0:/outputs \
bids/${CIRCLE_PROJECT_REPONAME} /bids_dataset /outputs group2 \
--license_file=/license.txt && \
mkdir -p ${HOME}/outputs1/ && \
sudo mv ${HOME}/data/ds114_test1_freesurfer_precomp_v6.0.0/00_group* ${HOME}/outputs1/ && \
cat ${HOME}/outputs1/00_group2_stats_tables/lh.aparc.thickness.tsv && \
cat ${HOME}/outputs1/00_group2_stats_tables/euler.tsv |
|
I think I it is "just" a path issue: trying to fix in 765a747 |
|
@Remi-Gau, it passed! Is there anything else we should fix for this PR? |
|
woohoo!!! I will merge and make a new release: this should trigger a push to docherhub. |
|
Wow, glad my ping triggered a new release! Thanks for all the hard work! Waiting on the dockerhub push and will make a PR to repronim, can't wait to try it out! |
FYI it is already on dockerhub but it is a bit of a mess at the moment: see here any thoughts on what would make most sense for users? |
I added Freesurfer 7 and some additional jobs on circleci to test and deploy it. Circlci config will likely need to be fixed before this is complete.