Skip to content
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

Export of group-level tables for aparc- and aseg-stats #26

Merged
merged 8 commits into from
Feb 13, 2017

Conversation

fliem
Copy link
Contributor

@fliem fliem commented Feb 9, 2017

My proposition for #19.

  • group is renamed group1
  • the table export is implemented as group2
  • exports tables to 00_group2_stats_tables folder in output_dir
  • 2to3-ed some freesurfer python scripts in the Dockerfile
  • added precomputed freesurfer folders (cross-sectional and longitudinal) to circleci to test group2

@fliem fliem mentioned this pull request Feb 9, 2017
run.py Outdated
if os.path.isdir(os.path.join(output_dir, "sub-" + s)):
subjects.append("sub-" + s)
else:
warn("No freesurfer subject found for %s in %s" % (s, output_dir))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

run.py Outdated
# create subcortical stats
table_file = os.path.join(table_dir, "aseg.tsv")
if os.path.isfile(table_file):
warn("Table file exists, delete if you want to recompute it. %s" % table_file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't recomputing be the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

run.py Outdated
'(in parallel) using the same output_dir.',
choices=['participant', 'group'])
'(in parallel) using the same output_dir. '
'"goup1" creates study specific group template. '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goup -> group

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

# make freesurfer python scripts python3 ready
RUN 2to3-3.4 -w $FREESURFER_HOME/bin/aparcstats2table
RUN 2to3-3.4 -w $FREESURFER_HOME/bin/asegstats2table
RUN 2to3-3.4 -w $FREESURFER_HOME/bin/*.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other option would be to install python2. is that you preference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem of this option is that it breaks the possibility to run the script outside of docker.
People without root privileges cannot run docker so it is a problem for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you want to run python scripts from inside the container using python interpreter from outside the container (host). How are you using this App without Docker? Are you using Singularity?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it is intended for but I just run the app by calling run.py from a server with freesurfer installed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. It's intended to be run via Docker or Singualrity. The latter works on clusters/hpc without root. You should check it out: http://singularity.lbl.gov/ http://bids-apps.neuroimaging.io/tutorial/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK but my cluster does not have singularity installed and if I install it myself with my user privileges I get this error when I try to execute the image:

-bash-4.1$ ./bids_example-2016-10-27-709845fcdcd0.img 
ERROR  : Singularity must be executed in privileged mode to use images
ABORT  : Retval = 255

Anyway it is not a big problem for me because I can easily modify this part of the script myself since everything else is working fine without the need to be in a container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend contacting your admins and asking them to install singularity system wide. It will make both of your life's easier!

@chrisgorgo
Copy link
Contributor

chrisgorgo commented Feb 9, 2017 via email

@fliem
Copy link
Contributor Author

fliem commented Feb 13, 2017

Is there anything else I can do here or on #24? Could you otherwise please create a new release? Thanks.

@chrisgorgo
Copy link
Contributor

I'm merging this - after tests pass on master I will make a new release.

@chrisgorgo chrisgorgo merged commit c2892d6 into bids-apps:master Feb 13, 2017
@fliem fliem deleted the grouptabs branch February 14, 2017 06:56
@fliem
Copy link
Contributor Author

fliem commented Feb 14, 2017

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants