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

FIX: Traverse BIDS hierarchy to find masks, bvals, and bvecs #587

Merged
merged 11 commits into from Nov 9, 2020

Conversation

richford
Copy link
Collaborator

@richford richford commented Nov 3, 2020

This PR traverses the BIDS hierarchy to find masks, bval files, and bvec files instead of assuming that they will be found in the same session. It is part bugfix, part enhancement because it also fixes the underscore error in PFTMask.get_mask().

Traversing the hierarchy should allow it to interact with common output patterns from nipreps.

@richford richford added bug enhancement effort: medium A medium amount of effort needed to resolve this issue impact: high High impact issues that will really improve the project labels Nov 3, 2020
@richford richford requested review from arokem and 36000 November 3, 2020 23:23
@pep8speaks
Copy link

pep8speaks commented Nov 3, 2020

Hello @richford! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-06 06:02:40 UTC

@richford
Copy link
Collaborator Author

richford commented Nov 4, 2020

@arokem and @36000, assuming the documentation builds pass, I think this is ready for review. In the latest commit, I changed all internal .bvals and .bvecs extensions to .bval and .bvec. As far as I know, the plural versions are not BIDS-compliant and they were making pybids mad.

@36000
Copy link
Collaborator

36000 commented Nov 4, 2020

I changed some of the nighlty smoke tests to run tractography when testing tractography related things. If this runs into emmory errors I can reduce number of seeds, but that should be in a different PR. These new tests would have been able to trigger your error, I think.

…(). get_nearest will still match the session entity and return the same session if it is indeed nearest. However, leaving it out of the explicit get_nearest() params, will allow masks that are not in the dwi session.
@arokem arokem merged commit 54e7d48 into yeatmanlab:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug effort: medium A medium amount of effort needed to resolve this issue enhancement impact: high High impact issues that will really improve the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants