Add --fs-no-resume option to reuse existing freesurfer outputs without resuming (eg. longitudinal pipeline base)

[bpinsard]

(Reopen of #371)

For now, it is not possible to run f/sMRIPrep with existing longitudinal freesurfer output.
If I understood correctly, the current surface generation workflow:

runs autorecon1 without skullstripping
injects a custom (and more robust) brainmask.mgz
resume to autorecon2/3 without any other tweaks.

The simpler way I found to input the base/template recon-all is to use the FreeSurferSource and reconnect everything appropriately. Does that make sense, or I missed something (like registering the sMRIPrep generated average to the freesurfer one).

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 78.54%. Comparing base (c72116a) to head (ba4611b).

Files	Patch %	Lines
smriprep/workflows/surfaces.py	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #393       +/-   ##
===========================================
+ Coverage   62.99%   78.54%   +15.54%     
===========================================
  Files          24       30        +6     
  Lines        1927     2093      +166     
  Branches      248      249        +1     
===========================================
+ Hits         1214     1644      +430     
+ Misses        678      417      -261     
+ Partials       35       32        -3     

Flag	Coverage Δ
ds005	∅ <ø> (?)
ds054	46.22% <20.00%> (?)
pytest	67.74% <40.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[effigies]

Okay, getting back to this. Thanks for the patience. As I understand it, instead of trying to correctly wrap longitudinal FreeSurfer in the same way we've been wrapping single-session FreeSurfer, this just give the user a "trust me, I know what I'm doing" button.

If that's the case, I think the better approach is probably to make the flag --fs-no-resume or something like that (open to alternatives), which would allow it to work with any FreeSurfer-like directory structure. This would work equally well with longitudinal FreeSurfer, fastsurfer (#280) or any other technique that comes out.

One thing that would be worth considering in that case is to provide a function to check for necessary files and error out early if they are not found. For example we need {lh,rh}.{white,pial,thickness,sulc,curv,sphere,sphere.reg} as well as T1.mgz, aparc.mgz, aparc+aseg.mgz and either FLAIR.mgz or T2.mgz, if T2w/FLAIR are detected.

That said, since this would be a "power user" feature, I don't think it's critical to write a hand-holder on the first pass.

[bpinsard]

Okay, getting back to this. Thanks for the patience. As I understand it, instead of trying to correctly wrap longitudinal FreeSurfer in the same way we've been wrapping single-session FreeSurfer, this just give the user a "trust me, I know what I'm doing" button.

Exactly, wrapping longitudinal pipeline is more complex.

If that's the case, I think the better approach is probably to make the flag --fs-no-resume or something like that (open to alternatives), which would allow it to work with any FreeSurfer-like directory structure. This would work equally well with longitudinal FreeSurfer, fastsurfer (#280) or any other technique that comes out.

Happy with renaming the flag, I had not thought about the fastsurfer usecase but that's a very good point.

One thing that would be worth considering in that case is to provide a function to check for necessary files and error out early if they are not found. For example we need {lh,rh}.{white,pial,thickness,sulc,curv,sphere,sphere.reg} as well as T1.mgz, aparc.mgz, aparc+aseg.mgz and either FLAIR.mgz or T2.mgz, if T2w/FLAIR are detected.

Such as a new nipype node that checks that the traits are not undefined?

That said, since this would be a "power user" feature, I don't think it's critical to write a hand-holder on the first pass.

[effigies]

Probably needs different line break points, but here's some updated wording:

Suggested change

	help='Import precomputed freesurfer without resuming '
	'(longitudinal or fastsurfer data) '
	"!expert option (you know what you're doing)!",
	help='EXPERT: Import pre-computed FreeSurfer reconstruction without resuming. '
	'The user is responsible for ensuring that all necessary files are present.'

[bpinsard]

Interestingly, templateflow/tpl-fsaverage#5 broke the CI run here (and likely any smriprep run?)
https://github.com/nipreps/smriprep/blob/master/smriprep/cli/run.py#L416 should be fixed? or niworkflows.utils.misc._copy_any ?
@oesteban

[effigies]

I guess we want

smriprep/smriprep/cli/run.py

Line 416 in 45c2e2f

dseg_tsv = str(api.get('fsaverage', suffix='dseg', extension=['.tsv']))

to become:

            dseg_tsv = str(api.get('fsaverage', hemi=None, seg='aparc', suffix='dseg', extension=['.tsv']))

[bpinsard]

I guess we want

smriprep/smriprep/cli/run.py

Line 416 in 45c2e2f

dseg_tsv = str(api.get('fsaverage', suffix='dseg', extension=['.tsv']))

to become:

            dseg_tsv = str(api.get('fsaverage', hemi=None, seg='aparc', suffix='dseg', extension=['.tsv']))

Opened #420
I guess this should be back-ported to the LTS as it will also pull from the latest templateflow version.

[effigies]

LGTM. Added a comment and updated the fmt:<on|off|skip> directives.