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

Support CombineDataFlag of 0 when averaging rotated bvecs #202

Merged

Conversation

MichielCottaar
Copy link
Contributor

@MichielCottaar MichielCottaar commented Dec 2, 2020

It seems like this was never actually supported

@MichielCottaar MichielCottaar mentioned this pull request Dec 2, 2020
@@ -114,7 +114,7 @@ else # Combining across diffusion directions with opposing phase-encoding polari

# Average Eddy-Rotated bvecs. Get for each direction the two b matrices, average those and then eigendecompose the average b-matrix to get the new bvec and bval.
# Also outputs an index file (1-based) with the indices of the input (Pos/Neg) volumes that have been retained in the output
${globalscriptsdir}/average_bvecs.py ${eddydir}/Pos.bval ${eddydir}/Pos_rotated.bvec ${eddydir}/Neg.bval ${eddydir}/Neg_rotated.bvec ${datadir}/avg_data ${eddydir}/Pos_SeriesVolNum.txt ${eddydir}/Neg_SeriesVolNum.txt
${globalscriptsdir}/average_bvecs.py ${eddydir}/Pos.bval ${eddydir}/Pos_rotated.bvec ${eddydir}/Neg.bval ${eddydir}/Neg_rotated.bvec ${datadir}/avg_data ${CombineDataFlag} ${eddydir}/Pos_SeriesVolNum.txt ${eddydir}/Neg_SeriesVolNum.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested that this works? Because based on the definition of main() in average_bvecs.py, it doesn't appear to me that we've properly provided inputs for bvalsoutfile, bvecsoutfile, and indicesoutfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've tested running this. The usage for the updated average_bvecs.py is:
usage: average_bvecs bvals1 bvecs1 bvals2 bvecs2 output_basename combine_data_flag [overlap1 overlap2]

This matches the way I run the script. bvalsoutfile, bvecsoutfile, and indicesoutfiles are defined based on the output_basename

Copy link
Contributor

@mharms mharms left a comment

Choose a reason for hiding this comment

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

It doesn't appear to me that the call to average_bvecs.py in eddy_postproc.sh is matched in terms of necessary arguments to the definition of main().

@@ -27,6 +27,7 @@ def main(bvals1file,
bvalsoutfile,
bvecsoutfile,
indicesoutfile,
only_combine,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused -- this code block here does not appear consistent with usage: average_bvecs bvals1 bvecs1 bvals2 bvecs2 output_basename combine_data_flag [overlap1 overlap2]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because python scripts don't start by calling the main function. The first code to be run is at the very end of the script (after the if __name__ == "__main__": on line 293). The code after that actually parses the arguments from the command line and defines bvalsoutfile, bvecsoutfile, and indicesoutfiles. Finally it calls the main function to do the actual work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the tutorial.

@mharms
Copy link
Contributor

mharms commented Dec 2, 2020

So, all testing is done, and this is ready to merge into #199 and from there into the master?

@MichielCottaar
Copy link
Contributor Author

I'm just rerunning the post_eddy script for the complete dataset (i.e., where we have the same amount of data for both phase encoding). I'm not expecting these changes to affect that output, but it would be good to check. I should know by tomorrow.

@MichielCottaar
Copy link
Contributor Author

The testing is done; this can be merged.

@glasserm glasserm merged commit 969ffc5 into Washington-University:dMRI_tweaks Dec 3, 2020
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