Mrtpipelines update #17

kaitj · 2022-11-02T23:02:24Z

Proposed changes

A number of quality changes (primarily to mrtpipelines.smk) to try to bring this up-to-date as of snakebids==0.6.2. Includes updates to:

pybids_inputs
Use of variables instead of grabbing from config for every rule
Renamed rules to reflect similar naming of mrtrix functions
Update mrtrix container to 3.0.3.
Make use of functools.partial

See the commit notes for full details.

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionalitiy)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Other (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

Changes have been tested to ensure that fix is effective or that a feature works.
Changes pass the unit tests
Code has been run through the poe quality task
I have included necessary documentation or comments (as necessary)
Any dependent changes have been merged and published

Notes

All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

- Add associated citation in comments - Renamed rules to match mrtrix function names - Updated mrtrix to latest release version - Updated pybids_inputs for diffusion - Updated names to more similarly match what was used in a different workflow (https://github.com/kaitj/mrtproc.git) - Updated notes to indicate whether TODO was for v0.1.x or v0.2.x - Define variables at top of .smk file to avoid grabbing from config for each rule - Fix remaining group directives that were missed

tkkuehn

I haven't actually tried running this yet, but a few comments from looking over the rules...

dbsc/workflow/rules/mrtpipelines.smk

- Replace config['derivatives'] with config['bids_dir'] and update join - Change config to get to return None if flags not defined - Remove unnecessary lambda calls - Update params for passing shell and lmax variables to command - Simplify shell commands - Add missing ',' for tck2connectome - Update param for defining node labels in connectome2tck - Update naming of filtered files to be more BIDSesque

- Redefined outputs to make use of wildcards to describe different tissue types - Kept responsemean call in one rule as the task it is performing is the same rather than split into separate rules - Consider using wildcards for tractograpy down the line

- Revert usage back to config["input_path"] - Set mrtrix_dir to make use of the output_dir rather than bids_dir

tkkuehn

A couple of things that I'm pretty sure will break the workflow, and some stylistic suggestions that you can take or leave depending on your preference. We should also put together some test data (even if just empty files) and make sure a dry-run of this completes before merging.

dbsc/config/snakebids.yml

dbsc/workflow/Snakefile

tkkuehn · 2022-11-04T12:51:15Z

dbsc/workflow/rules/mrtpipelines.smk

+
+
+# ------------ MRTRIX PREPROC BEGIN ----------#
+rule nii2mif:


It strikes me now that this is another rule that could be split into two. Not a huge benefit, but would allow a bit more parallelization. Up to you.

Agreed we could split this into two. I just figured to keep the two things that are doing the same thing together - the parallelization wouldn't speed up the workflow too much anyways.

tkkuehn · 2022-11-04T12:53:54Z

dbsc/workflow/rules/mrtpipelines.smk

-        sfwm=expand(rules.estimate_response.output.sfwm, subject=config['subjects']),
-        gm=expand(rules.estimate_response.output.gm, subject=config['subjects']),
-        csf=expand(rules.estimate_response.output.csf, subject=config['subjects'])
+        wm_rf=expand(rules.dwi2response.output.wm_rf, subject=config["subjects"]),


I missed this before, but I would think this should be expanding over input_zip_lists.

This was loosely based off of the workflow here: https://github.com/kaitj/mrtproc/blob/main/mrtproc/workflow/Snakefile

I'd probably hold off on making this change until we have a chance to test.

dbsc/workflow/rules/mrtpipelines.smk

tkkuehn · 2022-11-04T13:01:51Z

dbsc/workflow/rules/mrtpipelines.smk


-rule compute_tensor:
+
+rule dwi2tensor:


This is another rule that could be split into two.

Yes agreed, although again, not too much benefit and ultimately this could be an optional rule down the line (may not want tensor measures).

dbsc/workflow/rules/mrtpipelines.smk

- Changes `use_bids_inputs=False` in config - Removed group analysis level in `snakebids.yml` - Added `desc` as a wildcard in `snakebids.yml` - Added missing '&&' in mrtpipelines.smk

- Make use of functools.partial to iterate over different wildcards rather than expand

kaitj · 2022-11-04T21:18:44Z

I just went through and replaced any use of expand for the old outputs to make use of functools.partial as of d80dbe2

- Fixes pattern for and expands over node1 and node2 - Snakefmt fixes

kaitj added 8 commits November 2, 2022 18:05

Fix root for bids call, params.threads

177614d

Remove lmax param in dwi2fod rule

c313eb7

Update call for input path

455c8d8

Add flag for responsemean_dir

72b4a4d

Replace root dir with derivatives dir

1247ee4

Update pybids_inputs

f2b9e5a

Snakefmt updates

2a8f402

github-actions bot requested review from ataha24, Bradley-Karat and tkkuehn November 2, 2022 23:02

github-actions bot assigned kaitj Nov 2, 2022

tkkuehn requested changes Nov 3, 2022

View reviewed changes

kaitj force-pushed the mrtpipelines-update branch from 28fb0f9 to 9669f8a Compare November 3, 2022 17:16

Use wildcards for preproc rules

1e81fde

- Redefined outputs to make use of wildcards to describe different tissue types - Kept responsemean call in one rule as the task it is performing is the same rather than split into separate rules - Consider using wildcards for tractograpy down the line

kaitj requested a review from tkkuehn November 3, 2022 17:43

kaitj added 3 commits November 3, 2022 13:49

rm non-existant group target

62c1686

fix responsemean_flag if statement

a828d65

Make change back to config dict

0ddb5d2

- Revert usage back to config["input_path"] - Set mrtrix_dir to make use of the output_dir rather than bids_dir

kaitj added the maintenance Updates or improvements that do not change functionality of the code label Nov 3, 2022

tkkuehn requested changes Nov 4, 2022

View reviewed changes

kaitj force-pushed the main branch from b9e802a to 492a3b1 Compare November 4, 2022 16:09

kaitj added 2 commits November 4, 2022 13:31

Multiple fixes per tkuehn's comments

3df24f4

- Changes `use_bids_inputs=False` in config - Removed group analysis level in `snakebids.yml` - Added `desc` as a wildcard in `snakebids.yml` - Added missing '&&' in mrtpipelines.smk

Replace expand with functools.partial

d80dbe2

- Make use of functools.partial to iterate over different wildcards rather than expand

kaitj added 3 commits November 4, 2022 17:49

rm desc from wildcards

0ff97da

replace os.path.join with pathlib.Path

d59cb04

Remove unneeded suffix entity in bids_response_out call

5df79d5

kaitj force-pushed the mrtpipelines-update branch from daf4b3c to 5df79d5 Compare November 7, 2022 22:44

kaitj added 2 commits November 7, 2022 17:51

fix nodes params -> range(1,73)

06a6ee9

fix mrtpipelines, add to Snakefile

2f18507

- Fixes pattern for and expands over node1 and node2 - Snakefmt fixes

kaitj changed the base branch from main to smk-updates November 13, 2022 22:52

kaitj merged commit ffb5c94 into smk-updates Nov 13, 2022

kaitj deleted the mrtpipelines-update branch November 13, 2022 22:56

kaitj mentioned this pull request Nov 13, 2022

Rule updates #24

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mrtpipelines update #17

Mrtpipelines update #17

kaitj commented Nov 2, 2022 •

edited

Loading

tkkuehn left a comment

tkkuehn left a comment

tkkuehn Nov 4, 2022

kaitj Nov 4, 2022

tkkuehn Nov 4, 2022

kaitj Nov 4, 2022

tkkuehn Nov 4, 2022

kaitj Nov 4, 2022

kaitj commented Nov 4, 2022 •

edited

Loading



		# ------------ MRTRIX PREPROC BEGIN ----------#
		rule nii2mif:

Mrtpipelines update #17

Mrtpipelines update #17

Conversation

kaitj commented Nov 2, 2022 • edited Loading

Proposed changes

Types of changes

Checklist

Notes

tkkuehn left a comment

Choose a reason for hiding this comment

tkkuehn left a comment

Choose a reason for hiding this comment

tkkuehn Nov 4, 2022

Choose a reason for hiding this comment

kaitj Nov 4, 2022

Choose a reason for hiding this comment

tkkuehn Nov 4, 2022

Choose a reason for hiding this comment

kaitj Nov 4, 2022

Choose a reason for hiding this comment

tkkuehn Nov 4, 2022

Choose a reason for hiding this comment

kaitj Nov 4, 2022

Choose a reason for hiding this comment

kaitj commented Nov 4, 2022 • edited Loading

kaitj commented Nov 2, 2022 •

edited

Loading

kaitj commented Nov 4, 2022 •

edited

Loading