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: Output thickness, curvature, and sulcal depth files #296

Closed
wants to merge 11 commits into from

Conversation

ericfeczko
Copy link
Contributor

This address issue #288. I'm running into some errors with the outputs due to some syntax issues that were introduced. Will be doing some testing with circle CI today to debug. Will also ensure pep8 compatibility.

Copy link
Member

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

If we restore all of the removed lines, we'll preserve the work done in #295. Then I think we can reduce this to a relatively small addition. We'll then need to go into outputs.py and add a couple nodes to write out the files correctly, mirroring what we do for .surf.gii:

# Surfaces
name_surfs = pe.MapNode(
Path2BIDS(), iterfield="in_file", name="name_surfs", run_without_submitting=True
)
ds_surfs = pe.MapNode(
DerivativesDataSink(base_directory=output_dir, extension=".surf.gii"),
iterfield=["in_file", "hemi", "suffix"],
name="ds_surfs",
run_without_submitting=True,
)

Comment on lines +578 to +580
(fs2gii, allsurf_list, [('converted', 'in1')]),
(fscurv2funcgii, allsurf_list, [('converted', 'in2')]),
(allsurf_list, outputnode, [('out', 'surfaces')])
Copy link
Member

Choose a reason for hiding this comment

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

I think we're going to have trouble conflating .surf.gii files and .shape.gii files. I think we should probably not combine them and instead make a new morphometrics output. Something like:

Suggested change
(fs2gii, allsurf_list, [('converted', 'in1')]),
(fscurv2funcgii, allsurf_list, [('converted', 'in2')]),
(allsurf_list, outputnode, [('out', 'surfaces')])
(fscurv2funcgii, outputnode, [('converted', 'morphometrics')]),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes a lot of sense -- I'll make sure to do the same moving forward when handling the cifti conversion

Comment on lines +548 to +552
allsurf_list = pe.Node(
niu.Merge(7),
name="allsurf_list",
run_without_submitting=True,
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allsurf_list = pe.Node(
niu.Merge(7),
name="allsurf_list",
run_without_submitting=True,
)

Comment on lines +539 to +547
whitesurf_list = pe.Node(
niu.Merge(3, ravel_inputs=True),
name="whitesurf_list",
run_without_submitting=True,
)
fscurv2funcgii = pe.MapNode(
fs.MRIsConvert(out_datatype="gii", to_scanner=True),
iterfield=["in_file", "scalarcurv_file"], name="fscurv2funcgii",
)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to iterate if in_file is always the same.

Suggested change
whitesurf_list = pe.Node(
niu.Merge(3, ravel_inputs=True),
name="whitesurf_list",
run_without_submitting=True,
)
fscurv2funcgii = pe.MapNode(
fs.MRIsConvert(out_datatype="gii", to_scanner=True),
iterfield=["in_file", "scalarcurv_file"], name="fscurv2funcgii",
)
fscurv2funcgii = pe.MapNode(
fs.MRIsConvert(out_datatype="gii"),
iterfield=["scalarcurv_file"], name="fscurv2funcgii",
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

won't the in_file vary by hemisphere? I'm a little confused by the interface, I'll admit :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. That might be the best way to do this after all.

Comment on lines +573 to +576
(get_surfaces, whitesurf_list, [('smoothwm', 'in1'),
('smoothwm', 'in2'),
('smoothwm', 'in3')]),
(whitesurf_list, fscurv2funcgii, [('out', 'in_file')]),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(get_surfaces, whitesurf_list, [('smoothwm', 'in1'),
('smoothwm', 'in2'),
('smoothwm', 'in3')]),
(whitesurf_list, fscurv2funcgii, [('out', 'in_file')]),
(get_surfaces, fscurv2funcgii, [('smoothwm', 'in_file')]),

@effigies effigies added this to the 0.10.x milestone Jul 21, 2022
@effigies
Copy link
Member

@ericfeczko The requirement that we find the right ?h.white (or ?h.smoothwm) for this is pushing logic that should be into the interface into the workflow. I think a cleaner solution is to make a new interface that just sub-classes MRIsConvert and infers the correct in_file if scalarcurv is passed. WDYT?

@ericfeczko
Copy link
Contributor Author

that sounds perfect -- I'm in meeting hell right now, but I can try to give this a shot tonight or tomorrow.

@effigies
Copy link
Member

Here's an (untested) attempt: 857e0bd

You may need to add my fork to try it out:

git remote add effigies https://github.com/effigies/smriprep.git
git fetch effigies
git cherry-pick 857e0bd

@effigies
Copy link
Member

What are the expected outputs? I guess .dscalar.nii or .shape.gii?

@mgxd mgxd changed the title adding giftis for thickness curvature and sulcal depth ENH: Output thickness, curvature, and sulcal depth files Oct 14, 2022
@mgxd
Copy link
Collaborator

mgxd commented Oct 18, 2022

Closed by #305

@mgxd mgxd closed this Oct 18, 2022
@rosemccollum rosemccollum deleted the master branch May 31, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants