[ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces #3529

l-espana · 2022-11-30T13:14:30Z

Summary

Adds FreeSurfer longitudinal interfaces per issue #3345.

List of changes proposed in this PR (pull-request)

Add BaseReconAll to allow running recon-all with timepoints to create a base template.
Add LongReconAll to allow running recon-all for a session given a base template.

codecov · 2022-11-30T13:28:34Z

Codecov Report

Patch coverage: 40.74% and project coverage change: -0.02 ⚠️

Comparison is base (a9e25e1) 63.20% compared to head (78d580b) 63.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
- Coverage   63.20%   63.19%   -0.02%     
==========================================
  Files         308      308              
  Lines       40770    40792      +22     
  Branches     5480     5645     +165     
==========================================
+ Hits        25770    25779       +9     
  Misses      14000    14000              
- Partials     1000     1013      +13

Impacted Files	Coverage Δ
nipype/interfaces/freesurfer/preprocess.py	`63.88% <30.43%> (-0.54%)`	⬇️
nipype/interfaces/freesurfer/longitudinal.py	`93.33% <100.00%> (+0.18%)`	⬆️

... and 11 files with indirect coverage changes

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

ghisvail

This looks very good. I like your proposal for separating the base and long interfaces from the main one, which avoids overcharging the main interface which is already quite complex.

I was wondering whether you had tested a complete Nipype workflow for FS longitudinal composed of all three tasks? I have made a suggestion which could improve composition.

ghisvail · 2022-12-01T08:07:40Z

nipype/interfaces/freesurfer/longitudinal.py

+
+    >>> from nipype.interfaces.freesurfer.longitudinal import LongReconAll
+    >>> longrecon = LongReconAll()
+    >>> longrecon.inputs.long_id = ("ses-1","sub-template")


From a workflow construction perspective, providing long_id as tuple might make composition from all, base and then long interfaces more difficult.

In pydra-freesurfer, I have split it as longitudinal_timepoint_id and longitudinal_template_id and required the latter be specified if the former is. This way they are both exposed as separate input to the interface.

Oh, that's a very good point. I can separate those out. I tend to run each step separately, but I will make that change and test out a full workflow with all three steps when I can...should definitely make sure that works with this setup.

l-espana · 2022-12-14T13:15:24Z

Update on this: I was able to successfully run a nipype workflow with all longitudinal steps. (Not sure if I should document the relevant pieces of that somewhere). I think I can remove the [WIP], not sure if I need to update anything else or if anybody else has comments, perhaps @0rC0? I know you had worked on this previously as well...

ghisvail · 2022-12-14T13:22:17Z

Update on this: I was able to successfully run a nipype workflow with all longitudinal steps.

That's great.

Not sure if I should document the relevant pieces of that somewhere

One suggestion would be to post a working snippet here with like all 3 instances of ReconAll, BaseReconAll and LongReconAll wired together?

l-espana · 2022-12-14T14:04:52Z

This is hopefully enough to help people get started and should be close enough to what I actually ran...

from nipype.interfaces.freesurfer.preprocess import ReconAll
from nipype.interfaces.freesurfer.longitudinal import BaseReconAll, LongReconall
from niworkflows.engine.workflows import LiterateWorkflow as Workflow
from niworkflows.utils.bids import collect_data

bids_dir = "."
subject_data = collect_data(bids_dir, "sub-1")[0]
freesurfer_dir = "derivatives/freesurfer"
wf = Workflow(name="longitudinal_freesurfer")

initrecon = pe.MapNode(interface=ReconAll(directive="all", subjects_dir=freesurfer_dir), 
    name="initrecon", iterfield=["T1_files", "subject_id"])
initrecon.inputs.subject_id = ["ses-1","ses-2"]
initrecon.inputs.T1_files = subject_data["t1w"]

baserecon = pe.Node(BaseReconAll(base_id="sub-1", directive="all", subjects_dir=freesurfer_dir), 
    name="baserecon")

longrecon = pe.MapNode(LongReconAll(directive="all", subjects_dir=freesurfer_dir), 
    name="longrecon", iterfield=["long_id"])
longrecon.inputs.long_id = ["ses-1","ses-2"]

wf.connect([
    (initrecon, baserecon, [("subject_id", "timepoints")]),
    (baserecon, longrecon, [("subject_id", "base_id")]),
])

ghisvail · 2022-12-14T14:41:46Z

.zenodo.json

+    },
+    {
+      "affiliation": "Department of Neurosurgery, Medical College of Wisconsin",
+      "name": "Espana, Lezlie",
+      "orcid": "0000-0002-6466-4653"


I'd suggest to modify the zenodo file within its own commit. Also, I believe you should keep Satra as last author.

Oh of course, my mistake.

ghisvail

Thanks for this initial proposal. This first iteration looks very good and your snippet shows very well how it can be all composed.

I have a few suggestions wrt to namings, constraints and some typos.

nipype/interfaces/freesurfer/longitudinal.py

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

ghisvail · 2022-12-15T13:38:44Z

My suggestions did not include updates to the doctests, so they will likely fail. Could you update them please?

You will also need to refresh the test suite generated automatically from the specs.

l-espana · 2022-12-15T13:40:38Z

Yes, working on that now. :)

l-espana · 2022-12-15T14:14:28Z

I don't know that we can take out the subject_id from the input spec...We're inheriting from the ReconAll spec which has a default subject_id of "recon-all" so I think we need to override it. Unless there's another way to negate that?

Edit: Unless we turn off the "usedefault" in the original spec of course. Was just trying not to mess too much with the original.

ghisvail · 2022-12-22T08:08:49Z

@l-espana l-espana requested a review from ghisvail

I will have a look at it next week.

I don't know that we can take out the subject_id from the input spec

If we can't, then that may be a sign Base and Long specs should be merged into the main ReconAll with some appropriate xor logic. That's what I did for the Pydra interface and it worked.

effigies · 2022-12-27T16:27:20Z

Please merge master to fix the tests.

0rC0 · 2022-12-27T22:52:40Z

I was able to successfully run a nipype workflow with all longitudinal steps.
great! :-)

not sure if I need to update anything else or if anybody else has comments, perhaps @0rC0? I know you had worked on this previously as well...

It looks very good to me. At the beginning of the new year I can do some more tests.

l-espana · 2023-04-05T12:33:02Z

Hi! Any chance we can get this reviewed?

effigies · 2023-04-05T12:37:20Z

@ghisvail Were you reviewing this? Or would you like me to?

ghisvail

One last suggestion to improve the robustness of the interface.

The rest looks good to me. Nice work 👏

ghisvail · 2023-04-06T16:14:39Z

nipype/interfaces/freesurfer/preprocess.py

@@ -927,6 +930,29 @@ class ReconAllInputSpec(CommandLineInputSpec):
    )
    flags = InputMultiPath(traits.Str, argstr="%s", desc="additional parameters")

+    # Longitudinal runs


We might need to introduce additional constraints to avoid using cross-sectional specific arguments with base and long modes.

I am thinking of the T1_files, T2_file and FLAIR_file parameters at least. These should probably get a requirement on subject_id, which would achieve what we want thanks to your xor constraints.

That didn't cross my mind but excellent thought. I've add some requires and will test some variations next week to make sure that works as expected.

Looks good to me 👍

nipype/interfaces/freesurfer/preprocess.py

ghisvail

Looks good to be merged. I have highlighted a few formatting nuggets which black might complain about.

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

ghisvail · 2023-04-14T20:27:19Z

@effigies I don't have the privileges to merge this PR. Could you take care of it? 🙏

effigies · 2023-04-17T12:34:18Z

Looks like CI is broken; apologies. @ghisvail If you have time to look into this, it would be appreciated. Otherwise I'll get to it this week.

ghisvail · 2023-04-18T13:52:20Z

Looks like CI is broken; apologies. @ghisvail If you have time to look into this, it would be appreciated. Otherwise I'll get to it this week.

It looks like issues with CI started to surface on the last two merged PRs. CI complains that jobs no longer find black or pytest although they appear in the install step in the logs. The only reason I could think of is that the venv is no longer visible or activated between calls to the CI scripts (perhaps as a result of a change in GitHub Actions behaviour)?

Perhaps we could refactor CI to not use a dedicated venv? Maybe there are good reasons for it, I have just never seen it done in practice with GHA since each job is executed in a separate throwaway container anyway. It might also make things considerably simpler, as I find the current flow of CI scripts quite hard to follow. Let me know what you think.

effigies · 2023-07-05T19:58:16Z

Been a bit, but I got CI working (except CircleCI...) so hopefully any red checks you see are actionable.

l-espana added 2 commits November 29, 2022 13:54

Initial addition of FreeSurfer longitudinal interfaces.

da35d17

Style and testing.

d6ec3bd

l-espana marked this pull request as ready for review November 30, 2022 16:29

ghisvail reviewed Dec 1, 2022

View reviewed changes

l-espana added 2 commits December 2, 2022 07:36

Update LongReconAll to separate base template id.

0b51c54

Forgot check-before-commit. Update LongReconAll docstring.

6a294be

This was referenced Dec 3, 2022

Full freesurfer longitudinal output nipreps/fmriprep#2829

Closed

Longitudinal fmriprep with pre-run Freesurfer nipreps/smriprep#21

Open

l-espana changed the title ~~[WIP][ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces~~ [ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces Dec 14, 2022

Update longitudinal outputs to match separation of ids.

6971865

ghisvail reviewed Dec 14, 2022

View reviewed changes

Update/fix zenodo.

2d2072f

ghisvail suggested changes Dec 15, 2022

View reviewed changes

l-espana and others added 4 commits December 15, 2022 07:21

Remove subject_id input per code review

d38e98f

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

Make base recon inputs more explicit per code review

725cd82

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

Make longitudinal recon more explicit per code review

7d5253e

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

Fix typo

bdc39fc

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

l-espana requested a review from ghisvail December 20, 2022 16:26

l-espana added 2 commits December 28, 2022 13:16

Refactor longitudinal pipeline to live within ReconAll interface.

78a5feb

Black formatting.

c82b279

Fix list_outputs typo.

c52cf92

ghisvail suggested changes Apr 6, 2023

View reviewed changes

Additional xor/requires constraints

262bfb4

l-espana requested a review from ghisvail April 12, 2023 19:38

ghisvail reviewed Apr 14, 2023

View reviewed changes

nipype/interfaces/freesurfer/preprocess.py Outdated Show resolved Hide resolved

ghisvail reviewed Apr 14, 2023

View reviewed changes

nipype/interfaces/freesurfer/preprocess.py Outdated Show resolved Hide resolved

ghisvail approved these changes Apr 14, 2023

View reviewed changes

Apply formatting suggestions from code review

46e07bb

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>

effigies mentioned this pull request Apr 18, 2023

Reconsider CI management #3565

Open

Merge branch 'master' into iss3345

123ac53

effigies added 2 commits July 5, 2023 16:31

STY: Run black and fix typo

34a4ac6

TEST: make specs

78d580b

effigies force-pushed the iss3345 branch from 6de739c to 78d580b Compare July 5, 2023 20:43

effigies merged commit 68a7cd4 into nipy:master Jul 5, 2023

effigies mentioned this pull request Mar 20, 2024

REL: 1.8.7 #3639

Closed

6 tasks

effigies mentioned this pull request Oct 31, 2024

REL: 1.9.0 #3694

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces #3529

[ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces #3529

l-espana commented Nov 30, 2022

codecov bot commented Nov 30, 2022 •

edited

Loading

ghisvail left a comment •

edited

Loading

ghisvail Dec 1, 2022

l-espana Dec 1, 2022

l-espana commented Dec 14, 2022

ghisvail commented Dec 14, 2022

l-espana commented Dec 14, 2022

ghisvail Dec 14, 2022

l-espana Dec 14, 2022

ghisvail Dec 14, 2022

ghisvail left a comment

ghisvail commented Dec 15, 2022 •

edited

Loading

l-espana commented Dec 15, 2022

l-espana commented Dec 15, 2022 •

edited

Loading

ghisvail commented Dec 22, 2022

effigies commented Dec 27, 2022

0rC0 commented Dec 27, 2022

l-espana commented Apr 5, 2023

effigies commented Apr 5, 2023

ghisvail left a comment

ghisvail Apr 6, 2023

l-espana Apr 6, 2023

ghisvail Apr 14, 2023

ghisvail left a comment

ghisvail commented Apr 14, 2023

effigies commented Apr 17, 2023

ghisvail commented Apr 18, 2023

effigies commented Jul 5, 2023

[ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces #3529

[ENH] Issue 3345: Adding FreeSurfer longitudinal interfaces #3529

Conversation

l-espana commented Nov 30, 2022

Summary

List of changes proposed in this PR (pull-request)

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

ghisvail left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-espana commented Dec 14, 2022

ghisvail commented Dec 14, 2022

l-espana commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghisvail left a comment

Choose a reason for hiding this comment

ghisvail commented Dec 15, 2022 • edited Loading

l-espana commented Dec 15, 2022

l-espana commented Dec 15, 2022 • edited Loading

ghisvail commented Dec 22, 2022

effigies commented Dec 27, 2022

0rC0 commented Dec 27, 2022

l-espana commented Apr 5, 2023

effigies commented Apr 5, 2023

ghisvail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghisvail left a comment

Choose a reason for hiding this comment

ghisvail commented Apr 14, 2023

effigies commented Apr 17, 2023

ghisvail commented Apr 18, 2023

effigies commented Jul 5, 2023

codecov bot commented Nov 30, 2022 •

edited

Loading

ghisvail left a comment •

edited

Loading

ghisvail commented Dec 15, 2022 •

edited

Loading

l-espana commented Dec 15, 2022 •

edited

Loading