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

Updated cortical thickness and curvature regression #300

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

nagedem
Copy link
Contributor

@nagedem nagedem commented Sep 17, 2024

Bunch of new files and some changes in PostFreeSurfer.

nagedem and others added 26 commits April 9, 2024 17:18
Bunch of new scripts and some changes in postfreesurfer.
@@ -70,6 +70,7 @@ opts_AddOptional '--inflatescale' 'InflateExtraScale' 'number' "surface inflatio
opts_AddOptional '--processing-mode' 'ProcessingMode' 'HCPStyleData|LegacyStyleData' "disable some HCP preprocessing requirements to allow processing of data that doesn't meet HCP acquisition guidelines - don't use this if you don't need to" 'HCPStyleData'
opts_AddOptional '--structural-qc' 'QCMode' 'yes|no|only' "whether to run structural QC, default 'yes'" 'yes'
opts_AddOptional '--use-ind-mean' 'UseIndMean' 'YES or NO' "whether to use the mean of the subject's myelin map as reference map's myelin map mean, defaults to 'YES'" 'YES'
opts_AddOptional '--metric-regression' 'MetricReg' 'OLD, NEW or BOTH' "whether to use the updated curvature-thickness regression, defaults to 'B=BOTH'" 'B'
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want an option to not generate one of these? That would mean any later pipelines that use thickness and don't want curvature effects would have to be adapted to what the user chose for this setting.

I would think we want both to always be generated, without giving the user a choice to make their dataset more complicated for later handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

The old method is deprecated because it is substantially inferior. We are not using corrthickness much in pipelines (I think only the IDP pipeline) other than resampling it around.

Copy link
Member

Choose a reason for hiding this comment

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

The old version is easy to compute and not a large file. I don't see an obvious downside to keeping backward compatibility. I assume we don't want to use the old filename for the new output. What exactly are you suggesting by "deprecated"?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's purpose is entirely superseded by the new version.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, and because of that...what do you want the pipelines to do about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the current behavior, so we are providing a way to support that. We need to be able to make changes to already written pipelines. We can have transitional periods for deprecated features. corrThickness is obsolete with this new version. I don't really understand why this is so controversial. The pipelines need to be able to evolve with time.

Copy link
Member

Choose a reason for hiding this comment

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

Huh? I'm not saying the pipelines shouldn't change, where did you get that? It is controversial because you aren't providing a reason for your strong position.

Why does the user need an option to not run the new method? It doesn't overwrite a previously-released filename with different contents, right? So the new files can't interfere with any old code (unless the user happened to already generate files with those new names), right? If so, then why do we want an "old behavior" mode, when the new behavior doesn't change any files that the previous version generated? Why would a user want that?

Copy link
Member

@coalsont coalsont Sep 20, 2024

Choose a reason for hiding this comment

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

Is the concern that someone won't like the added runtime or dependencies from the new version?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is quite a bit longer.

Copy link
Member

Choose a reason for hiding this comment

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

People will generally only run PostFS after they have run FreeSurfer. How does it compare to the runtime of FreeSurfer?

Some simplification and cleaning;
Including -metric-palette, -cifti-palette, and -metric-mask;
Replacing "B" by "BOTH" in PostFreesurfer;
Changing map names
opts_AddMandatory '--subject-dir' 'SubjectDir' 'path' "folder containing all subjects"
opts_AddMandatory '--subject' 'Subject' 'subject ID' "subject-id"
opts_AddMandatory '--regname' 'RegName' 'my reg' "set the desired registration name(s) separated by @, 'string' 'RegName@RegName@[email protected].' use MSMSulc as default"
opts_AddOptional '--hemi' 'Hemi' 'hemisphere' "provide hemisphere for regression calculation, L=Left, R=Right, default 'L R' or B=Both" "B"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want the user to have the option not to run it on both hemispheres? Other scripts loop over a hardcoded L R.

Even if we keep it, the current description is disordered:

Suggested change
opts_AddOptional '--hemi' 'Hemi' 'hemisphere' "provide hemisphere for regression calculation, L=Left, R=Right, default 'L R' or B=Both" "B"
opts_AddOptional '--hemi' 'Hemi' 'hemisphere' "provide hemisphere for regression calculation, L=Left, R=Right, or B=Both, default 'B'" "B"

Added 'resample only' option.
Changed regname from mandatory to optional.
Fixed naming of variables and descriptions.
Added description to curvature script.
New folder for all python scripts
Sanity check for optional boolean argument
@glasserm glasserm self-requested a review October 1, 2024 00:19
Changed --no-computation to --skip-computation
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.

4 participants