Delete node dwi #766
Delete node dwi #766
Conversation
There was a problem hiding this comment.
Thanks @MatthieuJoulot !
Some comments as I was doing a first pass on it.
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
…d of self to epi_pipeline
There was a problem hiding this comment.
Thanks @MatthieuJoulot !
I still have some small concerns about the current implementation, especially around the marker parameter which hides some complexity behind a not very informative name.
I think we can make the code a bit more clear by passing to your delete function only what it needs (see comments below).
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @MatthieuJoulot !
I think this is much easier to understand now. 👍
I made some small suggestions to improve the documentation.
As a final step, I think we should add some unit tests for the two new functions that you have implemented (extract_sub_ses_folder_names for which you already have an example so should be straightforward, and delete_temp_dirs)
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_workflows.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @MatthieuJoulot !
The test looks good, I just think we can factorize a bit and use tmp_path (see comments below). Could you add a simple test for extract_sub_ses_folder_name too ?
clinica/pipelines/dwi_preprocessing_using_t1/dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
test/unittests/pipelines/dwi_preprocessing_using_t1/test_dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
test/unittests/pipelines/dwi_preprocessing_using_t1/test_dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
test/unittests/pipelines/dwi_preprocessing_using_t1/test_dwi_preprocessing_using_t1_utils.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for addressing my points @MatthieuJoulot !
This LGTM minus the current test failure due to the new parameter introduced in this PR (see my message below).
| sdc = epi_pipeline(name="SusceptibilityDistortionCorrection") | ||
| sdc = epi_pipeline( | ||
| self.base_dir, | ||
| self.parameters["delete_cache"], |
There was a problem hiding this comment.
Right now this line is breaking the instantiation tests (KeyError for delete_cache). I think we should add a default setting in check_pipeline_parameters (line 47), and maybe set delete_cache in the test.
|
@ghisvail let me know if you're planning to review this one when you're back (you're listed in the reviewers). Otherwise I'll merge it. |
Go for it |
|
Merging then ! |
The preprocessing using T1 for dwi currently has a problem: the amount of temporary data generated is way too important.
Thus, this PR aims at reducing this amount of temporary data, by suppressing it during the processing (when it is not useful anymore)