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

Add group directives to mrtpipelines.smk #12

Merged
merged 3 commits into from
Nov 2, 2022
Merged

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Oct 31, 2022

  • Splits the processing into 3 phases
    • Could consider splitting into 4 phases (an extra phase that separates tractography processing)
  • Refactor notes and TODOs
  • Remove "optional" GM exclusion for v0.1.0
  • Fix bash "if" conditional for dwi2fod
    • Conditions were for parameters were flipped (i.e. if empty vs non-empty)
  • Add mention of ideal "step size" for tractography

- Splits the processing into 3 phases
  - Could consider splitting into 4 phases (an extra phase that
separates tractography processing)
- Refactor notes and TODOs
- Remove "optional" GM exclusion for v0.1.0
- Fix bash "if" conditional for dwi2fod
  - Conditions were for parameters were flipped (i.e. if empty vs
non-empty)
- Add mention of ideal "step size" for tractography
@kaitj kaitj added the maintenance Updates or improvements that do not change functionality of the code label Oct 31, 2022
@kaitj kaitj added this to the v0.1.0 milestone Oct 31, 2022
@kaitj kaitj self-assigned this Oct 31, 2022
@kaitj kaitj requested a review from tkkuehn November 1, 2022 21:39
Copy link
Collaborator

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

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

A couple of comments that should be easy to fix, but looks good to merge after that!

- group
- participant2

# Mapping from analysis_level to set of target rules or files
targets_by_analysis_level:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to update this to reflect participant1 and participant2 analysis levels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this in c258563 - this is actually still suppose to be participant and group. Got this part mixed up with the group directive usage within the .smk file.

@@ -93,8 +94,8 @@ parse_args:
nargs: "+"

--step:
help: '(Mrtrix3) set the step size of the algorithm in mm
(default: 0.35mm)'
help: '(Mrtrix3) set the step size of the algorithm in mm. Should set to
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to include this recommendation it would be good to briefly explain why or give a reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added reasoning in 36529fd

Fixed participant levels to only have 'participant' and 'group'. Also
fixed the group directive within snakemake, which is separate from the
analysis levels.
@kaitj kaitj requested a review from tkkuehn November 2, 2022 14:57
@tkkuehn tkkuehn merged commit 5e11153 into main Nov 2, 2022
@tkkuehn tkkuehn deleted the mrtpipelines-groups branch November 2, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants