-
Notifications
You must be signed in to change notification settings - Fork 533
[ENH] Add all DIPY workflows dynamically #2905
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 #2905 +/- ##
=========================================
- Coverage 67.56% 67.5% -0.06%
=========================================
Files 343 344 +1
Lines 43604 43648 +44
Branches 5427 5438 +11
=========================================
+ Hits 29461 29466 +5
- Misses 13438 13471 +33
- Partials 705 711 +6
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.
Very nice! Just a couple of comments/questions from me.
nipype/interfaces/dipy/base.py
Outdated
return [(m, obj) for m, obj in inspect.getmembers(module) | ||
if inspect.isclass(obj) and | ||
issubclass(obj, module.Workflow) and | ||
m not in ['Workflow', 'CombinedWorkflow']] |
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.
As in dipy/dipy#1800, I suggest that you set this list to be a module-level constant.
BTW: the fact that you are using this same code there suggests that this might be a pattern worth including in the dipy library code, instead of here.
@@ -33,9 +34,19 @@ | |||
DTIModel = dipy_to_nipype_interface("DTIModel", ReconstDtiFlow) |
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.
Don't you want to remove these from here?
No problem @satra, I just did it! |
This PR is a follow up of #2830. It permits to add automatically all DIPY workflows following your DIPY version. This should fix this comment from @arokem and @chrisfilo.
Originally, the goal was to fix this issue but thanks to @mgxd who already did it!
I tag @Garyfallidis and @satra just to keep them updated.