Skip to content
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

Add a --fs-reuse-base option to reuse existing freesurfer outputs from a longitudinal pipeline #371

Closed
wants to merge 11 commits into from

Conversation

bpinsard
Copy link
Contributor

@bpinsard bpinsard commented Oct 2, 2023

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.

Currently, when reusing existing freesurfer outputs, it is not clear to me (maybe docs should be more informative) if the injected brainmask always triggers a rerun of autorecon2/3, from the nipype interface code, I would say no. So it seems only the brainmask.mgz is overwritten, but existing surfaces are directly grabbed.

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).

@pep8speaks
Copy link

pep8speaks commented Oct 2, 2023

Hello @bpinsard! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 247:80: E501 line too long (88 > 79 characters)
Line 249:80: E501 line too long (82 > 79 characters)
Line 250:80: E501 line too long (87 > 79 characters)
Line 261:80: E501 line too long (89 > 79 characters)
Line 263:80: E501 line too long (81 > 79 characters)

Comment last updated at 2023-10-05 17:47:57 UTC

@bpinsard bpinsard marked this pull request as ready for review October 5, 2023 14:51
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (6995f33) 58.65% compared to head (ef41799) 57.56%.
Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #371      +/-   ##
==========================================
- Coverage   58.65%   57.56%   -1.09%     
==========================================
  Files          26       26              
  Lines        1872     1909      +37     
  Branches      238      245       +7     
==========================================
+ Hits         1098     1099       +1     
- Misses        717      752      +35     
- Partials       57       58       +1     
Flag Coverage Δ
ds005 ∅ <ø> (∅)
ds054 46.51% <7.69%> (-0.28%) ⬇️
pytest 36.60% <0.00%> (-0.26%) ⬇️

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

Files Coverage Δ
smriprep/cli/run.py 66.80% <100.00%> (-7.08%) ⬇️
smriprep/workflows/anatomical.py 38.85% <ø> (ø)
smriprep/workflows/base.py 75.00% <ø> (ø)
smriprep/workflows/surfaces.py 21.36% <0.00%> (-1.16%) ⬇️

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

@bpinsard bpinsard changed the base branch from master to next October 20, 2023 19:50
@bpinsard
Copy link
Contributor Author

I am not super familiar with the circleci-codecov setup. Should I add an upload of the coverage.xml with a different tag after running with the new option to avoid the errors in the present PR? @effigies
Thanks for your feedback.

@effigies effigies deleted the branch nipreps:next November 20, 2023 00:47
@effigies effigies closed this Nov 20, 2023
@effigies
Copy link
Member

Sorry, this got closed due to the merge. I would not worry about codecov at the moment. I'm not sure what you mean about failures, unless you mean the red X's. It looks like it's getting submitted, it's just hard to get good coverage from a nipype workflow.

@bpinsard
Copy link
Contributor Author

No problem. Do you want me to reopen with base master? Or this change has been rejected?

@effigies
Copy link
Member

Yes, please reopen with master. It just got closed because I deleted the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants