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

Update handling of external dwi_dir #86

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Sep 12, 2023

Proposed changes

Resolves #84 - previously when dwi_dir was provided, the original bids_dir is still used to search for dwi data, resulting in a message regarding the inability to find files. This PR changes this behaviour, by splitting off the search for dwi data into a separate generate_inputs call, falling back to bids_dir if dwi_dir is not provided. By doing this, the message will only appear if there actually is no data that can be found. To that end, also added in the pybidsdb functionality, for both bids_dir and dwi_dir (if provided).

Additionally, there were many wildcards in the config previously, allowing potential expansion over a number of different entities (e.g. res, run, acquisition, etc). Instead this PR falls back to only allowing subject, session, and runs as wildcards, which I think makes a little more sense. In doing so, it allows for flexibility in the naming schemes to be picked up (e.g. prepdwi vs snakedwi) and in the event multiple inputs are found, an error is raised, suggesting to the end-user to make use of the --filter flag to constrain their selection. This also eliminates the need to hardcode the naming scheme of dwi that was done previously (e.g. desc=preproc or desc='eddy', res='orig'.

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 functionality)
  • 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.

Previously, if dwi_dir was passed, a naming scheme as assumed. Rather
than hardcode the naming scheme, this change passes dwi_dir through to
generate_inputs, which should enable different name schemes to be picked
up. Bonus, it suppresses the message of not being able to find files,
unless it actually is unable to find data in the provided paths. By
default, if dwi_dir is not provided, it falls back on bids_dir.
Previously, the usage of `wildcards` was vast in the config file,
allowing for wildcards for all sorts of entities. This PR constrains the
wildcards to just subject, session, and run (can consider others in the
future, but these make the most sense at the moment).

Dwi filters are constrained to be in the "T1w" space, as we assume dwi
processing should be performed in the same space as T1w. Alternatively,
can consider "orig" space as well, but will need to think about to
handle transformations to the T1w in that case.

If multiple entities are found (e.g. "res"), Snakebids will throw an
error reflecting the files found and suggest to the user to make use of
the `--filter` option to constrain the files.
@kaitj kaitj added the maintenance Updates or improvements that do not change functionality of the code label Sep 12, 2023
@kaitj kaitj force-pushed the maint/dwi_dir branch 2 times, most recently from bc01b4f to 57b43b6 Compare September 13, 2023 17:58
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 small comments but you can go ahead and merge after addressing them

scattr/workflow/Snakefile Show resolved Hide resolved
@kaitj kaitj merged commit e340824 into khanlab:main Sep 15, 2023
5 checks passed
@kaitj kaitj deleted the maint/dwi_dir branch September 15, 2023 14:39
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.

Don't warn about missing DWIs if --dwi_dir is given
2 participants