Issue #29 #30
Issue #29 #30
Conversation
run.py
Outdated
| @@ -430,7 +454,7 @@ def run(command, env={}, ignore_errors=False): | |||
| if os.path.isfile(table_file): | |||
| warn("Replace old file %s" % table_file) | |||
| os.remove(table_file) | |||
| cmd = "python3 `which aparcstats2table` --hemi {h} --subjects {subjects} --parc {p} --meas {m} " \ | |||
| cmd = "python2 `which aparcstats2table` --hemi {h} --subjects {subjects} --parc {p} --meas {m} " \ | |||
There was a problem hiding this comment.
Why is this switch necessary?
|
Well, in fact it has nothing to do with the issue of decomposing the steps, but I personally need this because I am unable to run docker in my cluster (nor singularity).
All that it costs is the necessity to install python 2 in addition to python 3, but the gain is to be compatible with the original FreeSurfer.
… Le 16 mai 2017 à 18:55, Chris Filo Gorgolewski ***@***.***> a écrit :
@chrisfilo commented on this pull request.
In run.py:
> @@ -430,7 +454,7 @@ def run(command, env={}, ignore_errors=False):
if os.path.isfile(table_file):
warn("Replace old file %s" % table_file)
os.remove(table_file)
- cmd = "python3 `which aparcstats2table` --hemi {h} --subjects {subjects} --parc {p} --meas {m} " \
+ cmd = "python2 `which aparcstats2table` --hemi {h} --subjects {subjects} --parc {p} --meas {m} " \
Why is this switch necessary?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Do you mind splitting those two set of modifications into two PRs? It will
make it easier to review and will almost certainly lead to faster merge for
one of them.
…On Tue, May 16, 2017 at 10:43 AM, Nicolas Traut ***@***.***> wrote:
Well, in fact it has nothing to do with the issue of decomposing the
steps, but I personally need this because I am unable to run docker in my
cluster (nor singularity).
All that it costs is the necessity to install python 2 in addition to
python 3, but the gain is to be compatible with the original FreeSurfer.
> Le 16 mai 2017 à 18:55, Chris Filo Gorgolewski ***@***.***>
a écrit :
>
> @chrisfilo commented on this pull request.
>
> In run.py:
>
> > @@ -430,7 +454,7 @@ def run(command, env={}, ignore_errors=False):
> if os.path.isfile(table_file):
> warn("Replace old file %s" % table_file)
> os.remove(table_file)
> - cmd = "python3 `which aparcstats2table` --hemi {h} --subjects
{subjects} --parc {p} --meas {m} " \
> + cmd = "python2 `which aparcstats2table` --hemi {h} --subjects
{subjects} --parc {p} --meas {m} " \
> Why is this switch necessary?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp5EuX3C-owthMhB8rmD9EDiiyAVuks5r6eA6gaJpZM4Ncx-2>
.
|
|
I see there is a problem with the path of the file |
|
There is no reason and it could be moved, but the Dockerfile and circle.yml would have to be adjusted. This also seems like a material for a separate PR ;) |
|
But I don't see any line which need to be modified in |
|
Actually I don't think you need to change anything in circle. Moving version or run.py should work equally well - don't have preference. |
run.py
Outdated
| parser.add_argument('--n_cpus', help='Number of CPUs/cores available to use.', | ||
| default=1, type=int) | ||
| parser.add_argument('--stages', help='Autorecon stages to run.', | ||
| choices=["autorecon1", "autorecon2", "autorecon2-cp", "autorecon2-wm", "autorecon-pial", "autorecon3", "autorecon-all", "all"], | ||
| default=["autorecon-all"], | ||
| nargs="+") | ||
| parser.add_argument('--steps', help='Longitudinal steps to run.', |
There was a problem hiding this comment.
Could we use human readable labels for the steps instead of numbers? It would be easier to understand what this option is for. Maybe: 'session', 'subject_template', 'long' ?
There was a problem hiding this comment.
yes, please. Or maybe cross instead of session to stick with freesurfer lingo?
|
This looks good to me, but there are a lot of changes. @fliem could you also have a look? |
There was a problem hiding this comment.
Thanks. Looks good.
Could you please add a test in circleci (e.g.) running the cross-sectional autorecon1 step for one session of ds114_test2
(available in ~/data/ds114_test2) - this also would close #23
In general, the code gets increasingly hard to read. We should think about how to make it more maintainable in the future.
run.py
Outdated
| parser.add_argument('--n_cpus', help='Number of CPUs/cores available to use.', | ||
| default=1, type=int) | ||
| parser.add_argument('--stages', help='Autorecon stages to run.', | ||
| choices=["autorecon1", "autorecon2", "autorecon2-cp", "autorecon2-wm", "autorecon-pial", "autorecon3", "autorecon-all", "all"], | ||
| default=["autorecon-all"], | ||
| nargs="+") | ||
| parser.add_argument('--steps', help='Longitudinal steps to run.', |
There was a problem hiding this comment.
yes, please. Or maybe cross instead of session to stick with freesurfer lingo?
run.py
Outdated
| input_args, | ||
| stages, | ||
| args.n_cpus) | ||
| # resume_cmd = "recon-all -base %s -sd %s %s -parallel -openmp %d"%(fsid, |
There was a problem hiding this comment.
Does getting rid of resume_cmd change the behavior?
|
ahhh. I think this is ready to merge, is it? |
No description provided.