Stand Alone MSMSulc.sh script

[nagedem]

No description provided.

[coalsont]

You didn't need to close the other PR, it will automatically update whenever you push. You have also added the merge commit and msmsulc.sh back into this PR.

Edit: but I have found that if you push upstream's master to the PR branch, github will close the PR on its own, unfortunately.

[glasserm]

The if conditional for computing the rotated sphere needs to be there or the entire block pulled out of MSMSulc and left in PostFreeSurfer. We should not be overwriting files that we make available in package downloads necessarily.

[coalsont]

I thought the idea was to replace the MSMSulc section of PostFS with a call to this script. Only a patch script should need a conditional for whether to generate a file, and patch scripts don't need to be in the repo.

[glasserm]

The purpose of this script is either to replace MSMSulc in the pipelines or to allow different MSMSulc to be run after the fact. The same script can do both.

[coalsont]

Putting the sphere.rot generation into PostFS outside of the MSMSulc script is a way to avoid overwriting, without using a conditional, but at the same time, the main reason we need the rotation is for MSM. While it appears to be used in MSMAll (side note, our "rotation" doesn't currently decompose the affine to a pure rotation, so theoretically it would be better and not exactly equivalent to provide the unrotated native sphere for cumulative distortion measurement in MSMAll, and we could even do this for MSMSulc too), since MSMAll is initialized by MSMSulc anyway, sphere.rot can be said to be purely an MSMSulc requirement, with MSMAll depending on MSMSulc, which would make the MSMSulc script/section the obvious place to generate it. I think I somewhat prefer this option (generate sphere.rot in PostFS immediately before calling MSMSulc.sh), not having conditional behavior on the filesystem state at all.

Alternatively, having the condition instead test "if the file doesn't exist, or the regname is MSMSulc, then generate it" may better emulate the expected/current behavior of rerunning PostFS/"MSMSulc" after something like faulty memory problems. Presumably the "after the fact" additional usage would generally be set to "MSMSulcStrain", and so "MSMSulc" can overwrite every time without issue (removing the surprise behavior change for at least the main pipeline). Presumably it doesn't matter whether a "longitudinal combined subject" folder overwrites this internally prior to release of longitudinal results (I don't know the longitudinal expected flow well enough to say whether it isn't even an issue).

If we don't expect people to want to manually run it "after the fact" with the MSMSulc name (which would overwrite downloaded things), then maybe the default should be MSMSulcStrain, and PostFS should provide the "magic" MSMSulc name when it calls it.

[glasserm]

We are using the rotated sphere for other things too. Let's just always run it in PostFreeSurfer so that we have it even if MSMSulc is not the registration we use.

[nagedem]

Just committed and pushed the relevant changes for your review.

[nagedem]

I have added the hemi argument. But I am not sure if you can see the commit.

…

On Thu, Apr 25, 2024 at 7:55 PM Tim Coalson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In PostFreeSurfer/scripts/FreeSurfer2CaretConvertAndRegisterNonlinear.sh <#290 (comment)> : > - - ${CARET7DIR}/wb_command -surface-distortion "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".sphere.native.surf.gii "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".sphere.MSMSulc.native.surf.gii "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".EdgeDistortion_MSMSulc.native.shape.gii -edge-method - - ${CARET7DIR}/wb_command -surface-distortion "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".sphere.native.surf.gii "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".sphere.MSMSulc.native.surf.gii "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".Strain_MSMSulc.native.shape.gii -local-affine-method - ${CARET7DIR}/wb_command -metric-merge "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainJ_MSMSulc.native.shape.gii -metric "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".Strain_MSMSulc.native.shape.gii -column 1 - ${CARET7DIR}/wb_command -metric-merge "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainR_MSMSulc.native.shape.gii -metric "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".Strain_MSMSulc.native.shape.gii -column 2 - ${CARET7DIR}/wb_command -metric-math "ln(var) / ln (2)" "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainJ_MSMSulc.native.shape.gii -var var "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainJ_MSMSulc.native.shape.gii - ${CARET7DIR}/wb_command -metric-math "ln(var) / ln (2)" "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainR_MSMSulc.native.shape.gii -var var "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".StrainR_MSMSulc.native.shape.gii - rm "$AtlasSpaceFolder"/"$NativeFolder"/"$Subject"."$Hemisphere".Strain_MSMSulc.native.shape.gii - - RegSphere="${AtlasSpaceFolder}/${NativeFolder}/${Subject}.${Hemisphere}.sphere.MSMSulc.native.surf.gii" + mkdir -p "$AtlasSpaceFolder"/"$NativeFolder"/MSMSulc + + cp "$AtlasSpaceFolder"/"$NativeFolder"/${Hemisphere}.sphere.rot.native.surf.gii "$AtlasSpaceFolder"/"$NativeFolder"/MSMSulc/${Hemisphere}.sphere_rot.surf.gii + + ./global/scripts/MSMSulc.sh --subject-dir="$StudyFolder" --subject="$Subject" --regname="$RegName" Need to add --hemi "$Hemisphere" to the arguments. — Reply to this email directly, view it on GitHub <#290 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMFYMLSVHZU73IFXTG45AODY7GQYTAVCNFSM6AAAAABF7T7OT2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRTHA2DQOBWGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[coalsont]

Looks like you didn't update your local clone after accepting my suggested edits on github, and then you made the same edits again manually. The merge seems to have figured out what you meant, so the current version is okay, but in the future, if you ever have to commit the same edits twice, something has gone wrong.

[nagedem]

Hmm, not really. I did not do the same edits manually again. I was able to pull the changes to local computer. But perhaps I needed to do that more than once (for each accepted change) before committing and pushing the changes in the other script.

…

On Thu, Apr 25, 2024 at 9:19 PM Tim Coalson ***@***.***> wrote: Looks like you didn't update your local clone after accepting my suggested edits on github, and then you made the same edits again manually. The merge seems to have figured out what you meant, so the current version is okay, but in the future, if you ever have to commit the same edits twice, something has gone wrong. — Reply to this email directly, view it on GitHub <#290 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMFYMLTBA5EU74RZ6J5NFNDY7G2UFAVCNFSM6AAAAABF7T7OT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZYGUYTAOJZG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[coalsont]

This commit appears to have several of the suggestions I made, which also have commits that look github-generated from accepting suggestions:

240e677

After accepting suggestions on github, you should git pull --ff-only in your local clone before making more commits. Any edits made using github end up as new commits that are only on github, and you have to tell your local clone to go look for updates.

[glasserm]

There are several issues that I identified. Please fix.

[nagedem]

I have just reversed the most recent changes.

…

On Sat, Apr 27, 2024 at 7:51 PM glasserm ***@***.***> wrote: ***@***.**** requested changes on this pull request. There are several issues that I identified. Please fix. — Reply to this email directly, view it on GitHub <#290 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMFYMLSL2J3KLSVFNLIXVIDY7RBYTAVCNFSM6AAAAABF7T7OT2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRWHA2TSMBRGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>