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

ENH: Project fsLR mesh onto native sphere to enable single-shot resampling into fsLR #339

Merged
merged 21 commits into from
Jun 1, 2023

Conversation

effigies
Copy link
Member

@effigies effigies commented May 18, 2023

This PR does a few things that enable resampling BOLD to fsLR using goodvoxels masking:

  1. Uses wb_command -surface-sphere-project-unproject to project the fsLR meshes onto the native space (creating what HCP calls sphere.reg.LR_reg. If I understand correctly, this is the file that will be replaced if we choose to use MSMSulc in the future.
  2. Temporarily adds some atlas files that are not in tpl-fsLR so we can use them ASAP. The names are kept consistent with HCP naming to avoid confusion. I had to md5sum HCP and templateflow files to tell what we already had or didn't have.
  3. Adds wb_command -surface-resample, although we do not use it here. We use it to generate an fsLR midthickness file, but since it's cheap to generate and we don't save it, it seems more appropriate to use it in fMRIPrep. Putting it here allows us to revise that decision if needed.
  4. Reimplements portions of FreeSurfer2CaretConvertAndRegisterNonlinear.sh and RibbonVolumeToSurfaceMapping.sh from DCAN-HCP.
  5. CHANGES smoothwm outputs to white. These are needed for workbench operations, and there's no clear reason to keep smoothwm. See Add white FreeSurfer surface to derivatives; replace smoothwm? #338.

Closes #338.

Copy link
Member Author

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgxd This is ready for review.

.circleci/ds005_outputs.txt Outdated Show resolved Hide resolved
smriprep/interfaces/surf.py Show resolved Hide resolved
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall, just some minor comments.

One thing - this adds fsLR sphere registration as required step, even on runs that do not wish to do --cifti-output / --project-goodvoxels. If the processing time this adds is negligible, I'm fine with it, but if not we might want to add someway to avoid this. Or alternative, add some notion of a --fast-mode which will avoid non-essential steps.

.circleci/ds005_outputs.txt Outdated Show resolved Hide resolved
smriprep/interfaces/workbench.py Outdated Show resolved Hide resolved
smriprep/interfaces/workbench.py Outdated Show resolved Hide resolved
smriprep/interfaces/workbench.py Outdated Show resolved Hide resolved
smriprep/workflows/outputs.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2023

Codecov Report

Patch coverage: 69.29% and project coverage change: +0.54 🎉

Comparison is base (c06497e) 53.59% compared to head (8361310) 54.14%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
+ Coverage   53.59%   54.14%   +0.54%     
==========================================
  Files          20       22       +2     
  Lines        1364     1461      +97     
  Branches      229      242      +13     
==========================================
+ Hits          731      791      +60     
- Misses        578      614      +36     
- Partials       55       56       +1     
Flag Coverage Δ
ds005 ∅ <ø> (∅)
ds054 44.96% <38.59%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
smriprep/conftest.py 0.00% <ø> (-100.00%) ⬇️
smriprep/interfaces/cifti.py 52.54% <ø> (ø)
smriprep/utils/bids.py 47.00% <ø> (ø)
smriprep/workflows/outputs.py 46.51% <0.00%> (-1.11%) ⬇️
smriprep/workflows/surfaces.py 11.97% <12.50%> (+0.05%) ⬆️
smriprep/interfaces/surf.py 35.61% <48.00%> (+3.61%) ⬆️
smriprep/workflows/anatomical.py 45.56% <50.00%> (-0.28%) ⬇️
smriprep/interfaces/conftest.py 91.30% <91.30%> (ø)
smriprep/data/__init__.py 94.44% <94.44%> (ø)
smriprep/interfaces/workbench.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@effigies
Copy link
Member Author

If the processing time this adds is negligible, I'm fine with it

5 seconds.

@effigies effigies force-pushed the enh/surface_sphere_project branch from c579384 to 8361310 Compare June 1, 2023 11:13
@effigies
Copy link
Member Author

effigies commented Jun 1, 2023

@mgxd Okay to merge?

@mgxd mgxd merged commit 0cd77e3 into nipreps:master Jun 1, 2023
@effigies effigies added this to the 0.12.0 milestone Jun 2, 2023
@effigies effigies deleted the enh/surface_sphere_project branch June 5, 2023 11:51
effigies added a commit to nipreps/fmriprep that referenced this pull request Jun 6, 2023
…3011)

## Changes proposed in this pull request

1) Remove goodvoxels and CIFTI-specific code from `mri_vol2surf`
resampling workflow. fsnative and fsaverage will remain the same for
now.
2) Removes `fsaverage` as an implicit resampling target for
`--cifti-output`. `fsaverage` and `fsLR` are now entirely independent.
3) Create a subject-specific cortical ROI mask in GIFTI space. Not
currently output, as it's only needed internally; it's not a great mask
but is intended to be more liberal than the fsLR mask applied later.
4) Adds a number of `wb_command` commands, including an OpenMP mixin
that allows `OMP_NUM_THREADS` to be set in the environment. I think most
commands can take advantage, but I only tagged slow ones (take >10s)
that I could verify actually used them.
5) Depends on nipreps/smriprep#339 and
nipreps/niworkflows#806.


TODO:

* [x] Restore literate workflow code.
* [x] Restore saving goodvoxels mask, if desired.

## Documentation that should be reviewed

TODO

---

Closes #2990.
@edickie edickie mentioned this pull request Jul 20, 2023
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.

Add white FreeSurfer surface to derivatives; replace smoothwm?
3 participants