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

Yash refactor #254

Merged
merged 21 commits into from
Dec 19, 2023
Merged

Yash refactor #254

merged 21 commits into from
Dec 19, 2023

Conversation

ythackerCS
Copy link
Contributor

Finished refactoring PreFresurfer ready for others to test I did do one run seems to get through without variable errors

@glasserm
Copy link
Contributor

glasserm commented Mar 20, 2023

Let's not change the setup script or the Batch script.

@ythackerCS
Copy link
Contributor Author

ythackerCS commented Mar 20, 2023 via email

@glasserm
Copy link
Contributor

I wouldn't have committed those two files to this pull request. I don't know the best way to back that out, perhaps Tim can help.

@glasserm
Copy link
Contributor

Can you make a side by side comparison of the usage statements when you run the script without arguments/trigger the usage?

@coalsont
Copy link
Member

I have removed the changes to other files. Please check that the changes to the pipeline are all of the changes you intended to commit.

@ythackerCS
Copy link
Contributor Author

PreFreeSurfer_new.txt
PreFreeSurfer_old.txt
Here are the help's of the two scripts old and new.

@glasserm
Copy link
Contributor

Thanks. I noticed a new parameter at the end for the new script (perhaps the last line of the old was just omitted) and also there are some places were there are too many spaces in the sentences, e.g.: "for hcp-style" has two spaces instead of one.

@coalsont
Copy link
Member

I believe the old script accepted --usejacobian, but didn't list it in the help info:

UseJacobian=`opts_GetOpt1 "--usejacobian" $@`

@glasserm
Copy link
Contributor

I see. Perhaps @demsarjure or @sivcek have some ideas on acceptance testing in their environment we could do to thoroughly test all the options?

{
if [ ! $HCP_DEBUG_USETRAP == 'OFF' ]; then
{
if [[ "$HCP_DEBUG_USETRAP" == "ON" ]]; then
Copy link
Member

@coalsont coalsont Mar 21, 2023

Choose a reason for hiding this comment

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

FYI, these edits are from the branch I told him to use. This PR probably shouldn't go into master, but into the debug-function-enable branch. Once all of the pipelines have gone through testing, we can rebase that branch onto master.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think we want to do this expeditiously, as Misha needs to use the updated structural pipelines for longitudinal pipeline development.

Copy link
Member

Choose a reason for hiding this comment

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

Will Misha need all 3 structural pipelines converted before starting? If so, we should still do them into the debug-function-enable branch, test them with that change, and then I can rearrange the commits so the new versions are on master, debug-function-enable is based on that, and the rest of the PRs can continue the same way.

Even if he needs each one as soon as they are available, I can do that kind of rearrangement 3 times without much trouble, so that we don't have to test them twice (without, and then with) the debug.shlib changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be best if he had all three pipelines converted before doing any development on them. There is also the longitudinal pipeline. Perhaps I can convince Jure to convert that one too.

@coalsont
Copy link
Member

The extra spaces look like they came from where the old help info had newlines. There is a style choice here, do we convert it into sentences by adding a period, or do we put in newlines, making it longer but with less line wrapping towards the start of each sentence? Looks like I kept the newlines when I converted hcp_fix_multi_run, but I don't care that much.

@glasserm
Copy link
Contributor

I would rather we didn't have double spaces in the middle of sentences. I think letting your code do the wrapping is fine.

@demsarjure
Copy link
Contributor

I see. Perhaps @demsarjure or @sivcek have some ideas on acceptance testing in their environment we could do to thoroughly test all the options?

I can build a develop QuNex container and acceptance test the refactored FS on our end. This acceptance test runs HCP MPP over 7 different datasets (3 HCP and 4 internal, from Siemens, GE and Phillips scanners).

@glasserm
Copy link
Contributor

Would that test your legacy options thoroughly? Both SE and standard fieldmaps?

@demsarjure
Copy link
Contributor

One of the datasets is legacy style, it has SE pairs though, but is not multi-band.

If there is something else/bespoke to test, we can discuss it.

@glasserm
Copy link
Contributor

Well for now this is just HCP Structural Preprocessing and PreFreeSurfer. It would be good to test a dataset with standard field maps, another with SE fieldmaps, and any legacy options you guys find important.

@demsarjure
Copy link
Contributor

OK, just to be clear the idea is to test PreFreeSurfer on the Yash_Refactor branch. We should test it on the standard data, SE fieldmaps and other possible legacy options.

What is the timeline here? If we do this by the end of next week, would this work?

@glasserm
Copy link
Contributor

Yes, that's fine.

@glasserm
Copy link
Contributor

@demsarjure were you able to do these tests?

@demsarjure
Copy link
Contributor

@glasserm

Sorry, for not getting back to you sooner!

The command is not backwards compatible, what QuNex generates right now is missing a number of mandatory parameters. So it seems like many parameters that previously had defaults set in your scripts are now mandatory. See below for details. Please advise about how to proceed. The agreement we have is that we set only mandatory parameters QuNex side, while optional ones can be set through QuNex but use HCP defaults if not set.

/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/PreFreeSurfer/PreFreeSurferPipeline.sh --path="/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp" --subject="10159" --t1="/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp/10159/unprocessed/T1w/10159_T1w_MPR1.nii.gz" --t2="NONE" --t1template="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm.nii.gz" --t1templatebrain="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain.nii.gz" --t1template2mm="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm.nii.gz" --t2template="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm.nii.gz" --t2templatebrain="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm_brain.nii.gz" --t2template2mm="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_2mm.nii.gz" --templatemask="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain_mask.nii.gz" --template2mmmask="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm_brain_mask_dil.nii.gz" --brainsize="150" --fnirtconfig="/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/config/T1_2_MNI152_2mm.cnf" --seechospacing="NONE" --seunwarpdir="NONE" --t1samplespacing="NONE" --t2samplespacing="NONE" --unwarpdir="NONE" --gdcoeffs="NONE" --avgrdcmethod="NONE" --processing-mode="LegacyStyleData"

Test file: 
/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp/10159/MNINonLinear/T1w_restore_brain.nii.gz
------------------------------------------------------------

Tue Apr 11 02:06:19 EDT 2023:PreFreeSurferPipeline.sh: While running '/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/PreFreeSurfer/PreFreeSurferPipeline.sh --path=/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp --subject=10159 --t1=/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp/10159/unprocessed/T1w/10159_T1w_MPR1.nii.gz --t2=NONE --t1template=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm.nii.gz --t1templatebrain=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain.nii.gz --t1template2mm=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm.nii.gz --t2template=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm.nii.gz --t2templatebrain=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm_brain.nii.gz --t2template2mm=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_2mm.nii.gz --templatemask=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain_mask.nii.gz --template2mmmask=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm_brain_mask_dil.nii.gz --brainsize=150 --fnirtconfig=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/config/T1_2_MNI152_2mm.cnf --seechospacing=NONE --seunwarpdir=NONE --t1samplespacing=NONE --t2samplespacing=NONE --unwarpdir=NONE --gdcoeffs=NONE --avgrdcmethod=NONE --processing-mode=LegacyStyleData':
Tue Apr 11 02:06:19 EDT 2023:PreFreeSurferPipeline.sh: While running '/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/PreFreeSurfer/PreFreeSurferPipeline.sh --path=/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp --subject=10159 --t1=/gpfs/gibbs/pi/n3/Studies/QuNexAcceptTest/Results/jd2528/qunex_container/QuNex_acceptance_CNP_BIDS_20230310_030316/sessions/10159/hcp/10159/unprocessed/T1w/10159_T1w_MPR1.nii.gz --t2=NONE --t1template=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm.nii.gz --t1templatebrain=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain.nii.gz --t1template2mm=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm.nii.gz --t2template=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm.nii.gz --t2templatebrain=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_0.7mm_brain.nii.gz --t2template2mm=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T2_2mm.nii.gz --templatemask=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_0.7mm_brain_mask.nii.gz --template2mmmask=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/templates/MNI152_T1_2mm_brain_mask_dil.nii.gz --brainsize=150 --fnirtconfig=/gpfs/gibbs/pi/n3/software/HCP/HCPpipelines/global/config/T1_2_MNI152_2mm.cnf --seechospacing=NONE --seunwarpdir=NONE --t1samplespacing=NONE --t2samplespacing=NONE --unwarpdir=NONE --gdcoeffs=NONE --avgrdcmethod=NONE --processing-mode=LegacyStyleData':
Tue Apr 11 02:06:19 EDT 2023:PreFreeSurferPipeline.sh: ERROR: missing the following mandatory parameter(s): --fmapmag --fmapphase --fmapgeneralelectric --echodiff --SEPhaseNeg --SEPhasePos --topupconfig
Tue Apr 11 02:06:19 EDT 2023:PreFreeSurferPipeline.sh: ERROR: missing the following mandatory parameter(s): --fmapmag --fmapphase --fmapgeneralelectric --echodiff --SEPhaseNeg --SEPhasePos --topupconfig
Tue Apr 11 02:06:19 EDT 2023:PreFreeSurferPipeline.sh: ABORTING

@glasserm
Copy link
Contributor

@ythackerCS @coalsont can you look at this? The goal here was not to change the requirements of the command, but just to simplify the I/O interface code.

@demsarjure
Copy link
Contributor

Several parameters above should not be mandatory as they are connected with acquisition devices and protocols (e.g., fmapgeneralelectric, fmapphase, SEPhaseNeg). I think the issue might be just that some parameters were erroneously labeled as mandatory instead of as optional.

@ythackerCS
Copy link
Contributor Author

ythackerCS commented Apr 11, 2023 via email

@ythackerCS
Copy link
Contributor Author

ythackerCS commented Apr 12, 2023 via email

@demsarjure
Copy link
Contributor

Hi @ythackerCS ,

Yes, I am waiting for the FreeSurfer refactor as that one is more similar to Longitudinal FreeSurfer than this one and it will save me a bunch of work. But if there is a conversion script then I will use that. Where can I find this script, I am unable to find it in this correspondence.

Thanks, Jure

@coalsont
Copy link
Member

In the old-style code, the mandatory/optional logic is sometimes implemented deep in the processing code, in this case the fieldmap stuff is passed as-is to the T2wToT1wDistortionCorrectAndReg.sh sub-script and it deals with whether the arguments are valid. The --avgrdcmethod option is probably the only fieldmap-related option that is mandatory.

For cases like this, maybe we could borrow QuNex's understanding of what is mandatory vs optional?

@ythackerCS
Copy link
Contributor Author

ythackerCS commented Apr 12, 2023 via email

@coalsont
Copy link
Member

So, PreFS is also one of the oldest scripts, and may be less consistent than others about the help info.

I also notice that the way to tell it not to do distortion correction is currently "any string that isn't recognized", which is not good design (not typo-safe). I think we should use something like "NONE" here to mean no distortion correction (though this is different than how other scripts unfortunately use "NONE" to represent an empty list), and the *) case should be an error of "unrecognized distortion correction method". This also suggests validating the value of --avgrdcmethod much earlier in the script, before it calls any sub-scripts or does any other processing.

@coalsont coalsont marked this pull request as ready for review December 13, 2023 00:48
@coalsont coalsont merged commit 15a9b0e into master Dec 19, 2023
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.

5 participants