fMRISurface related pull request #136
Conversation
1/ Changing tabs to spaces, removal of trailing line spaces 2/ Addition of `--mppversion` parameter and related check / validation code
|
For clarity: since there is no internal change to this pipeline for legacy data, is the purpose of adding the flag just so that the QuNex framework doesn't have to keep track of which pipelines have optional legacy behavior? |
|
The flag can be omitted it is just consistent now across pipelines and it opens up a possibility of adding changes later on. |
|
Also, if the user sets the "legacy" mppmode for the study, the logs from all the pipelines will be marked appropriately. |
|
The purpose of the flag is to allow the hcp pipelines to be run on legacy datasets that are missing certain inputs. Thus, I don't think it is expected that we will incrementally add more options controlled by this flag, because the inputs to some pipelines are only the outputs of other pipelines, so they should never need such an option. A user running the pipelines manually may not edit all the legacy options to enable them in scripts that run without them. If we want to ensure the log records if the user has used any legacy options for a subject, the main way I can think of is to create a file in the subject directory when a legacy option is used, permanently marking that subject and generating logging messages as such (and we shouldn't worry about users that might try to trick the detection). |
|
I'll just remove it then. It should be obvious from the other logs what was run. And if somebody wants to check them, they can. |
|
In the interest of irritating Grega with back and forth changes (:P) I would actually at this point class RegName="FS" into legacy mode both in PostFreeSurfer and here, as we really want to discourage using FreeSurfer registration now that MSMSulc is widely available and we have shown it to be substantially better in multiple publications. |
|
So, reclassifying FS registration would break the idea of this option being only used for aspects of the input data. If we say it is always harmful compared to MSMSulc on all data (what about tumor lesions, AVM, etc?), then it would fall into my category of "should not be added to the pipelines at all", however since it has already been available, removing it entirely may be unneccesarily unfriendly. In my ideal world, if we wanted to change it to be an error by default, we would have two separate options: one to control whether to allow never-recommended legacy features of the pipeline (FS reg), and one to allow features of the pipeline that can be appropriate on legacy data (no T2 image). This would draw a clear distinction between "making the best of what you have" and "choosing to do something bad to your data", and produce specific messages for each case. If we want to have only one option to control these two different categories of behaviors, which by definition is more confusing, then arguably it should use somewhat generic terminology. |
|
To be clear, I think entirely removing the code supporting the FS registration might be an acceptable solution, because if someone wants to match what they did previously on other data, best practice is to use the older release of the pipelines for consistency. |
|
I suppose we could instead add a third option to the existing flag for this situation, something like |
|
Tim's 3 option solution seems reasonable. I do like the idea of a recommended path for legacy data and further excluding non-recommended options. Here is a warning for FS registrattion (to be put in both fMRISurface and PostFreeSurfer): WARNING: FreeSurfer registration is deprecated in the HCP Pipelines as it results in poorer cross-subject functional and cortical areal alignment relative to MSMSulc. Additionally, FreeSurfer registration results in dramatically higher surface distortion (both isotropic and anisotropic). These things occur because FreeSurfer's registration has too little regularizattion of folding patterns that are imperfectly correlated with function and cortical areas, resulting in overfitting of folding patterns. See Robinson et al 2014, 2018 Neuroimage, and Coalson et al 2018 PNAS for more details. |
|
Usually when something is "deprecated" there is plenty of advance warning, but it continues to work "as is". The path consistent with that approach would be to add a warning message/statement, but leave RegName=FS functional without needing to invoke a special "mode" to use it. |
|
Given the issues with FS being used in the container for HCP processing despite clear instructions not to, I think we need to be more aggressive and force the user's attention on this one. |
|
Can this be the compromise: |
|
That would be my preferred approach, and more consistent with the usual path for deprecating a feature. |
|
Ok. Then all should be ready. |
GenericfMRISurfaceProcessingPipeline.shrelated changes1/ Cosmetic changes
Changing tabs to spaces, removal of trailing line spaces
2/ Addition of
--mppversionparameter and related check / validation codeGenericfMRISurfaceProcessingPipeline_1res.shrelated changes1/ Cosmetic changes
Changing tabs to spaces, removal of trailing line spaces
2/ Addition of
--mppversionparameter and related check / validation code