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
Statistics-Volume pipeline #54
Conversation
Hello @arnaudmarcoux! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-02-28 15:44:05 UTC |
optional.add_argument("-tup", "--threshold_uncorrected_pvalue", | ||
type=float, default=0.001, | ||
help='Threshold to display the uncorrected p-value ' | ||
+ '(--threshold_uncorrected_pvalue 0.001).') | ||
|
||
optional.add_argument("-tcp", "--threshold_corrected_pvalue", | ||
type=float, default=0.05, | ||
help='Threshold to display the corrected p-value ' | ||
'(default: --threshold_corrected_pvalue 0.05)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these flags are either not used or have wrong help message. Is this a copy/paste from statistics-surface
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really know what else to say about them (and yes it is a copy paste :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Simply remove these lines, there is no equivalent in your code.
clinica/pipelines/statistics_volume/statistics_volume_pipeline.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/statistics_volume/statistics_volume_pipeline.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/statistics_volume/statistics_volume_pipeline.py
Outdated
Show resolved
Hide resolved
@mdiazmel : In this PR, the @arnaudmarcoux: There are several flavours of the ICBM template. Could you tell me why you chose this version compared to another variant? If there is no particular reason, I would be in favor of using the one in |
clinica/pipelines/statistics_volume_correction/statistics_volume_correction_pipeline.py
Show resolved
Hide resolved
clinica/pipelines/statistics_volume_correction/statistics_volume_correction_utils.py
Outdated
Show resolved
Hide resolved
@@ -68,7 +66,7 @@ class {{ pipeline.class_name }}CLI(ce.CmdParser): | |||
# BIDS and/or CAPS directory as inputs. If the BIDS directory is not needed | |||
# for your pipeline, simply remove: | |||
# bids_directory=self.absolute_path(args.bids_directory), | |||
pipeline = T1FreeSurfer( | |||
pipeline = {{ pipeline.class_name }}( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently fixed in PR #63 .
clinica/pipelines/statistics_volume/statistics_volume_pipeline.py
Outdated
Show resolved
Hide resolved
Hi @arnaudmarcoux, I finished a first reading of your code. We are quite close to the final version :) I noticed that instanciation tests are missing. Moreover, I would advice you to move the content of Btw, it seems that outputs are not temporarly saved in working directory. Coud you tell me if this is the case or not? I will do the feedback regarding the Wiki tomorrow. Alex |
71ba51d
to
7afc88f
Compare
Currently, the output console looks like:
The first line will disappear when In if len(self.subjects):
print_images_to_process(self.subjects, self.sessions)
cprint('The pipeline will last a few minutes. Images generated by SPM will popup during the pipeline.')
print_begin_image('group-' + self.parameters['group_id']) (Adapted from |
Indeed, pipeline disk usage is determined by:
Done
Done
Everything is working on working dirs, except last node of statistics-volume-correction. I did not have a simple method to save the outputs, so I did not use a datasink. That's why there is no output node. |
) | ||
pipeline.run(plugin='MultiProc', plugin_args={'n_procs': 4}, bypass_check=True) | ||
compare_folders(join(root, 'out'), join(root, 'ref'), 'caps') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing:
# Remove data in out folder
clean_folder(join(root, 'out', 'caps'), recreate=False)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
assert np.allclose(nib.load(output_t_stat).get_data(), | ||
nib.load(ref_t_stat).get_data()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing:
# Remove data in out folder
clean_folder(join(root, 'out', 'caps'), recreate=False)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
When running
Why do we have 5 *report.png files ? We should only have 2: Besides, I made some mistakes when specifying the CAPS:
Finally:
It should be
Open question:
|
Final request: can you move the content of |
Space estimation was given in |
I don't remember how this was set...
2 different contrasts group 1 > group 2 and group 2 > group 1 Which makes 4 reports (5 because one of them is more than 1-page long. Moreover It is not possible to link each report to its task.
Done
I dont really have an opinion |
Ok. We'll put that aside.
Ok. Since FWEp is performed in |
To be followed in PR #67. |
This PR adds 2 new pipelines : statistics-volume and statistics-volume-correction