-
Notifications
You must be signed in to change notification settings - Fork 14
ENH: Precomputed derivatives #173
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
- Coverage 32.54% 32.21% -0.34%
==========================================
Files 44 44
Lines 3650 3703 +53
==========================================
+ Hits 1188 1193 +5
- Misses 2462 2510 +48
Continue to review full report at Codecov.
|
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.
Haven't gone through the JLF stuff yet. Some initial reactions, though.
coregistration_wf = init_coregistration_wf( | ||
omp_nthreads=omp_nthreads, | ||
sloppy=sloppy, | ||
debug="registration" in config.execution.debug, | ||
) | ||
coreg_report_wf = init_coreg_report_wf( | ||
output_dir=output_dir, | ||
) |
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.
Do we not still need these, even if there is a mask?
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.
since the T2w is only being used to generate the mask, nope. I was actually planning on removing the T2w restriction if a precomputed mask is available.
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.
Are we not outputting the coregistered T2w image? I think that's been a desired feature for fMRIPrep for a while, if it hasn't been explicitly brought up here.
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.
no we are not (yet)
This is ready for a final review - @hough129 @madisoth how has testing gone on your end? I'm working on a branch that implements |
I am still struggling to get it to fully complete - getting closer though. I believe @madisoth is having a separate issue that he reported on an issue thread. |
Are you running into an error?
Yes, though I believe its unrelated to this PR |
Yes - but I have a fix - the segmentation needs to be reproduced in order to test it. That will take a day or two. |
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
this is the final straw
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.
Overall this makes sense to me; I don't have the attention span to really check the connect statements in detail. Do we have a test case for all the major branches?
Not yet, but I'm hoping to beef up the test suite dramatically once this is in. |
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Adds
--derivatives
flag to allow for the use of precomputed derivatives (organized as one or more BIDS derivatives)Initial support is intended to allow the use of:
bold mask(Pushed to a later PR, as this will require bigger workflow refactor than the other two options)Addresses #166