-
Notifications
You must be signed in to change notification settings - Fork 532
ENH: Add new dwifslpreproc
interface for MRtrix3
#3278
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
Seems like the CI is failing at
|
I've restarted the CI... |
Codecov Report
@@ Coverage Diff @@
## master #3278 +/- ##
==========================================
+ Coverage 64.69% 64.73% +0.04%
==========================================
Files 302 302
Lines 39837 39869 +32
Branches 5286 5288 +2
==========================================
+ Hits 25773 25810 +37
Misses 12974 12974
+ Partials 1090 1085 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
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.
Thanks! This looks good overall. Some comments and at least one question.
|
||
class DWIPreprocInputSpec(MRTrix3BaseInputSpec): | ||
in_file = File( | ||
exists=True, argstr="%s", position=-10, mandatory=True, desc="input DWI image" |
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 these position
arguments really necessary for all inputs?
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.
Not necessary, this was my lazy way of ensuring the interface command stayed consistent with the examples from the mrtrix documentation.
I think if position is left empty, the inputs appear right after dwifslpreproc
? I was going for a format of dwifslpreproc in_file out_file -rpe_options -other_options
to follow the examples here.
Is there a way to achieve that without specifying positions for everything?
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.
If that's necessary, I think you could set position of 0 for in_file
, 1 for out_file
, 2 for rpe_options
, and leave it unset for the rest. If it's not actually necessary for an option, I would personally leave it unset, because you're indicating to future maintainers that they need to be very particular about this.
dwifslpreproc
interface for MRtrix3dwifslpreproc
interface for MRtrix3
Moved `-rpe_` to `argstr` for simpler input. Co-authored-by: Chris Markiewicz <[email protected]>
Fixed typos and improved `-eddy` and `-topup` optional inputs. Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Xihe Xie <[email protected]>
Looks like you're going to need to re- |
LGTM. Thanks! |
Summary
Added new MRtrix3
dwifslpreproc
interface.Close #3276
List of changes proposed in this PR (pull-request)
interfaces/mrtrix3/preprocess.py
namedDWIPreproc
, interfaces with the newly updateddwifslpreproc
command.make specs
created new pytestAcknowledgment