-
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
Quality (actions, pre-commit) and tests #31
Conversation
Will assign a reviewer when new PRs are made or when marked ready for review. Any reviews required after initial will need to be triggered by user.
- Added opionated yaml linter to pre-commit hook and github action - Requires shell command to filter and exclude snakemake and snakebids files - Run yamlfix on all other yaml files
- Add gitignore - Add bids_dir - Adds necessary derivatives - dwi is doubled to perform two different tests (one for dwi_dir flag) - freesurfer directories / files
- ensure paths passed through are absolute (exception for testing) - fix workflow issues
- add poe task for testing --responsemean_dir flag - fixed responsemean call (expansion over subjects)
- Created check_dir_path function to check if user passed directories are absolute paths. Throw an error otherwise - Replace existing checks in freesurfer and mrtpipelines with function.
- Added dwi_flag for preprocessed data - Added check to make sure absolute path is passed - Added test data within prepdwi directory for testing purposes
- Added tpl and updated names for MNI152NLin6Asym - Renamed MNI2009b -> MNI152NLin2009bAsym - Removed unnecessary files - Update config
- Update config to include tsv for subcortical labels
- Update freesurfer tsv to match bids spec - Update file name of Freesurfer dseg tsv
- Add inputs to rule all for copying freesurfer and zona tsv files to appropriate location
- Still need to fix piping of workflow, update entities, and filenames
- Adds in rule for labelmerge (needs updating with container and proper command) - Update resources with correct names and paths - Add tsv to rule all for copying from resource to bids folders for labelmerge - Simplify rule all
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.
Looks good and the tests run through on my machine
Will merge once we get #30 merged into |
Labelmerge
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.
After merging labelmerge
, the test_dwi
task fails (in the actions workflow and locally for me).
Proposed changes
This PR consolidates the changes first introduced in #18, adding quality actions and tasks. Additionally, adds dry-run testing for different flag options, namely to ensure the additional directory flags do not cause any issues for the workflow.
Note that this PR only ensure the dry-runs can pass without issues.
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.