-
Notifications
You must be signed in to change notification settings - Fork 3
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
Rule updates #24
Rule updates #24
Conversation
freesurfer.smk: - add exist_ok for Path.mkdir - fix duplicate input / out entries zona_bb_subcortex.smk: - add exist_ok for Path.mkdir - syntax and piping fixes - remove unnecessary include: mrtpipelines.smk: - add exist_ok for Path.mkdir - syntax and piping fixes - remove unnecessary include: - NOTE: Switch to new BidsInput (requires some refactoring; save for later PR) Snakefile: - add rule all - run snakefmt
Syntaxes and workflow bugs have been fixed as of c5b4856. Also did a dry-run through the workflow to ensure it works (although |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of generally minor changes but I think this is very close to being ready to merge
322ec1b
to
a346d43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick and one small change
- Add shebang to run.py - Use pathlib to get resource files (uses Path.parent instead of workflow.basedir/../) - Get rid of directory creation in freesurfer.smk - Add CLI arg to pass in freesurfer directory location - Update note regarding assumed preprocessing steps - Remove space constraint on T1w image
@tkkuehn Above changes should be fixed. I also found a few other workflow piping issues (namely incorrect use of wildcards in certain rules) that need to be corrected for when I was adding in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny comment but this is otherwise good to merge, so I don't need to review it again after
Proposed changes
This PR pulls in and resolves conflicts from #17, #20, and #21. It also resolves previous review comments. Most of these include quality changes. Briefly (click drop toggle to see notes from previous PRs for respective workflows):
Freesurfer
Mrtpipelines
zona_bb_subcortex
All rules have also been updated to make use of
functools.partial
.An additional note - a couple of files needed for transformation is too large to be shared via this repository and after a brief discussion, look into downloading it as part of the container build / point users to download location. Need to discuss with @jclauneuro to see what best option is here (files are SNSX32 related, namely T1w and subcortical segmentations in MNI152NLin6Asym and MNI152Nlin2009b space)
Types of changes
What types of changes does your code introduce? Put an
x
in the boxes that applyChecklist
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!poe quality
taskNotes
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.