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: Add sphere registration to fit workflow, check for precomputed #370

Merged
merged 15 commits into from
Oct 11, 2023

Conversation

effigies
Copy link
Member

@effigies effigies commented Sep 29, 2023

Not much to say. Moved some of the surface conversion into first-order fit, even though they are easy to calculate post hoc because they are needed if msmsulc is used. Initially tried to make some of it conditional, but it made it annoying to construct the complement after the fact. A few extra surfaces in the anatomical derivatives are not our biggest problems, space-wise.

@effigies effigies added the next label Sep 29, 2023
@pep8speaks
Copy link

pep8speaks commented Sep 29, 2023

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

Line 30:80: E501 line too long (91 > 79 characters)

Line 293:80: E501 line too long (90 > 79 characters)
Line 316:80: E501 line too long (91 > 79 characters)
Line 592:80: E501 line too long (83 > 79 characters)
Line 601:80: E501 line too long (97 > 79 characters)
Line 607:80: E501 line too long (96 > 79 characters)
Line 608:80: E501 line too long (93 > 79 characters)
Line 752:80: E501 line too long (82 > 79 characters)
Line 774:80: E501 line too long (89 > 79 characters)
Line 796:80: E501 line too long (94 > 79 characters)
Line 797:80: E501 line too long (84 > 79 characters)
Line 798:80: E501 line too long (81 > 79 characters)
Line 898:80: E501 line too long (86 > 79 characters)
Line 925:80: E501 line too long (87 > 79 characters)
Line 996:80: E501 line too long (80 > 79 characters)
Line 1096:80: E501 line too long (91 > 79 characters)
Line 1097:80: E501 line too long (93 > 79 characters)
Line 1110:80: E501 line too long (90 > 79 characters)
Line 1115:80: E501 line too long (80 > 79 characters)
Line 1117:80: E501 line too long (83 > 79 characters)
Line 1133:80: E501 line too long (94 > 79 characters)
Line 1144:80: E501 line too long (94 > 79 characters)
Line 1156:80: E501 line too long (93 > 79 characters)
Line 1158:80: E501 line too long (85 > 79 characters)
Line 1162:80: E501 line too long (80 > 79 characters)
Line 1167:80: E501 line too long (91 > 79 characters)
Line 1179:80: E501 line too long (91 > 79 characters)
Line 1181:80: E501 line too long (85 > 79 characters)
Line 1204:80: E501 line too long (97 > 79 characters)
Line 1208:80: E501 line too long (87 > 79 characters)
Line 1228:80: E501 line too long (85 > 79 characters)
Line 1229:80: E501 line too long (94 > 79 characters)
Line 1233:80: E501 line too long (95 > 79 characters)
Line 1237:80: E501 line too long (80 > 79 characters)
Line 1238:80: E501 line too long (83 > 79 characters)
Line 1257:80: E501 line too long (91 > 79 characters)
Line 1258:80: E501 line too long (93 > 79 characters)
Line 1262:80: E501 line too long (91 > 79 characters)
Line 1266:80: E501 line too long (85 > 79 characters)
Line 1267:80: E501 line too long (81 > 79 characters)

Line 369:80: E501 line too long (82 > 79 characters)

Line 355:80: E501 line too long (88 > 79 characters)
Line 734:80: E501 line too long (87 > 79 characters)
Line 773:80: E501 line too long (85 > 79 characters)
Line 782:80: E501 line too long (82 > 79 characters)
Line 799:80: E501 line too long (89 > 79 characters)

Comment last updated at 2023-10-02 18:08:57 UTC

@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

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

Comparison is base (f6b842f) 59.13% compared to head (3b5b230) 58.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #370      +/-   ##
==========================================
- Coverage   59.13%   58.44%   -0.70%     
==========================================
  Files          26       26              
  Lines        1845     1860      +15     
  Branches      235      238       +3     
==========================================
- Hits         1091     1087       -4     
- Misses        698      716      +18     
- Partials       56       57       +1     
Flag Coverage Δ
ds005 ∅ <ø> (∅)
ds054 46.49% <16.00%> (-0.34%) ⬇️
pytest 36.50% <2.00%> (-0.68%) ⬇️

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

Files Coverage Δ
smriprep/workflows/base.py 75.00% <0.00%> (ø)
smriprep/workflows/surfaces.py 22.62% <0.00%> (+0.30%) ⬆️
smriprep/utils/bids.py 46.87% <12.50%> (+8.87%) ⬆️
smriprep/workflows/outputs.py 53.01% <30.76%> (-0.97%) ⬇️
smriprep/workflows/anatomical.py 38.85% <14.47%> (-5.89%) ⬇️

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

@effigies
Copy link
Member Author

@mgxd I believe this is ready for review if you've got time.

@effigies
Copy link
Member Author

The current failure relates to passing BIDS filenames to Path2BIDS. We expect rh.thickness_converted.shape.gii and instead get sub-01_hemi-R_thickness.shape.gii because it's being found in the output directory. Need to think about how whether to bypass the extraction or make Path2BIDS more flexible.

@effigies
Copy link
Member Author

effigies commented Oct 2, 2023

This is passing. Codecov is complaining about coverage because coverage is bad.

Comment on lines +1099 to +1101
needed_anat_surfs = ["white", "pial", "midthickness"]
needed_metrics = ["thickness", "sulc"]
needed_spheres = ["sphere_reg", "sphere"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like a good time for sets

Suggested change
needed_anat_surfs = ["white", "pial", "midthickness"]
needed_metrics = ["thickness", "sulc"]
needed_spheres = ["sphere_reg", "sphere"]
needed_anat_surfs = {"white", "pial", "midthickness"}
needed_metrics = {"thickness", "sulc"}
needed_spheres = {"sphere_reg", "sphere"}

Copy link
Member Author

@effigies effigies Oct 4, 2023

Choose a reason for hiding this comment

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

Sets can vary in order with each interpreter's initial hash seed, so I'd worry about this causing some node to consider its inputs changed. The n is so small here that I think efficiency is not a significant concern.

Comment on lines +1114 to +1115
surfs = [surf for surf in needed_anat_surfs if surf not in found_surfs]
spheres = [sphere for sphere in needed_spheres if sphere not in found_surfs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

then we should just be able to do

Suggested change
surfs = [surf for surf in needed_anat_surfs if surf not in found_surfs]
spheres = [sphere for sphere in needed_spheres if sphere not in found_surfs]
surfs = list(needed_anat_surfs - found_surfs.keys())
spheres = list(needed_spheres - found_surfs.keys())

]),
])
# fmt:on
metrics = [metric for metric in needed_metrics if metric not in found_surfs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
metrics = [metric for metric in needed_metrics if metric not in found_surfs]
metrics = list(needed_metrics - found_surfs.keys())

@effigies
Copy link
Member Author

@mgxd It would be nice to depend on this. Is your previous suggestion blocking?

@mgxd
Copy link
Collaborator

mgxd commented Oct 11, 2023

No, it is not blocking

@effigies effigies merged commit 92773f5 into nipreps:next Oct 11, 2023
14 of 16 checks passed
@effigies effigies deleted the enh/fit-msmsulc branch October 11, 2023 20:08
effigies added a commit to nipreps/fmriprep that referenced this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants