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

replace the old _BC block with MyelinMap_BC #259

Merged
merged 12 commits into from
Mar 30, 2023
Merged

replace the old _BC block with MyelinMap_BC #259

merged 12 commits into from
Mar 30, 2023

Conversation

cyang31
Copy link
Contributor

@cyang31 cyang31 commented Mar 26, 2023

Background

There remain several legacy _BC code block that is not replaced by the module MyelinMap_BC.sh.

Main changes

  • replace the old _BC code block in MSMAll.sh with MyelinMap_BC.sh
  • CreateMyelinMaps.sh used gifti based _BC which is now replaced by the cifti-based MyelinMap_BC.sh, while the gifti files are kept
  • add arguments to PostFreeSurferPipeline.sh to adapt the module MyelinMap_BC.sh
  • update the example launch script PostFreeSurferPipelineBatch.sh
  • change the argument --myelin-target-file as a single argument with absolute file path that doesn't depend on --msm-all-templates

Evaluation

  • MSMAllPipeline logs with no error:

  • /media/myelin/alex/MSMAll_optimization/MSMAllPipeline.sh.e49165

  • /media/myelin/alex/MSMAll_optimization/MSMAllPipeline.sh.o49165

  • PostFreeSurferPipeline logs with no error:

  • /media/myelin/alex/MSMAll_optimization/PostFreeSurferPipeline.sh.e162416

  • /media/myelin/alex/MSMAll_optimization/PostFreeSurferPipeline.sh.o162416

@cyang31 cyang31 changed the title replace the old _BC block in MSMAll with MyelinMap_BC replace the old _BC block with MyelinMap_BC Mar 27, 2023
@cyang31 cyang31 marked this pull request as ready for review March 27, 2023 02:57
@coalsont
Copy link
Member

Should we also edit the _1res version while we are at it?

@cyang31
Copy link
Contributor Author

cyang31 commented Mar 28, 2023

I didn't find lines in CreateMyelinMaps_1res.sh that are performing the same operations in _BC module. It seems that the gifti files to construct cifti ${MyelinMap}_BC are already generated elsewhere. Maybe @glasserm can confirm it?

@coalsont
Copy link
Member

I said that before I looked at it, yes it seems _1res it just resamples the existing native mesh maps, which makes sense.

@@ -63,12 +63,15 @@ opts_AddMandatory '--lowresmesh' 'LowResMeshes' 'number' "usually '32', the stan
opts_AddMandatory '--subcortgraylabels' 'SubcorticalGrayLabels' 'file' "location of FreeSurferSubcorticalLabelTableLut.txt"
opts_AddMandatory '--freesurferlabels' 'FreeSurferLabels' 'file' "location of FreeSurferAllLut.txt"
opts_AddMandatory '--refmyelinmaps' 'ReferenceMyelinMaps' 'file' "high-resolution group myelin map to use for bias correction"
opts_AddMandatory '--msm-all-templates' 'MSMAllTemplates' 'path' "path to directory containing MSM All template files, e.g. 'YourFolder/global/templates/MSMAll'"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an odd variable name for PostFreeSurfer that precedes MSMAll by quite a few lines. I am not a huge fan of tying the myelin target file to a specific folder either. I would have preferred a full path and a single argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the myelin target as a single argument with an absolute path wherever the MyelinMap_BC is involved.

@glasserm
Copy link
Contributor

I didn't write the 1res code. That is a patch for if someone does not want to rerun PostFreeSurfer but wants to add a new lowres mesh resolution.

@cyang31
Copy link
Contributor Author

cyang31 commented Mar 29, 2023

tested without error: /media/myelin/alex/MSMAll_optimization/PostFreeSurferPipeline.sh.e2761221 and /media/myelin/alex/MSMAll_optimization/PostFreeSurferPipeline.sh.o2761221

@coalsont
Copy link
Member

Looks reasonable to me, and I think @glasserm's comment was addressed.

@coalsont coalsont merged commit 79b8b61 into Washington-University:master Mar 30, 2023
@cyang31 cyang31 deleted the fix_MSMAll0 branch March 30, 2023 23:54
@coalsont coalsont mentioned this pull request Apr 6, 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.

3 participants