-
Notifications
You must be signed in to change notification settings - Fork 95
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
[REF] Merge CLI into workflows #100
Conversation
This could make it easier to update/add workflows. Instead of having to manage two files, we just need to manage one (although we still need to update the parser in step with the workflow and its docstring.
We'll need the CI passing before merging, but I don't have a strong feeling on this reorganization either way. I can see the appeal of it, though, and am happy to move forward if it makes things more intuitive ! I imagine it will also make @rmarkello happy, since he thinks we have too many modules 😄 |
Workflows can now take in a filename for data (i.e., a string) in addition to a list of files.
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
=========================================
- Coverage 45.81% 45.42% -0.4%
=========================================
Files 29 28 -1
Lines 1602 1585 -17
=========================================
- Hits 734 720 -14
+ Misses 868 865 -3
Continue to review full report at Codecov.
|
@@ -28,7 +27,7 @@ def compare_nifti(fn, test_dir, res_dir): | |||
""" | |||
res_fp = (res_dir/fn).as_posix() | |||
test_fp = (test_dir/fn).as_posix() | |||
assert np.allclose(nb.load(res_fp).get_data(), nb.load(test_fp).get_data()) | |||
assert np.allclose(nib.load(res_fp).get_data(), nib.load(test_fp).get_data()) |
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 consistently using nib
instead of nb
, now ? I just want to make sure we don't end up with conflicting imports !
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.
I can change it back, but my understanding was that nib
was the alias used in the nibabel documentation (e.g., here). On the other hand, while they may use that alias in at least some of their documentation, I've never seen anything official about it, so we can use nb
if you prefer.
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.
It looks like that's our only import of it for now, and I know I'm the minority opinion so happy to change :) Just wanted to check !
This LGTM ! Ready to merge, @tsalo ? |
I think so, unless anyone has any concerns. |
I think @rmarkello is most likely to have feedback here (since he was involved in the original organization decisions) ! |
Love it! Thrilled to see consolidation of these modules. On a relatively cursory glance it looks good to me, but if you'd like me to do a line-by-line I can. It seems like @emdupre already checked it over though, so I'm happy to see it merged. |
💯 Merging, then ! Thanks everyone ! |
This reorganization would make it easier to update workflows, as the argument parser and the workflow function are consolidated in one file for each workflow (instead of spread between
tedana.cli
andtedana.workflows
.Changes proposed in this pull request:
tedana.cli
totedana.workflows
. Each workflow submodule now contains a function named[name]_workflow
(e.g.,tedana_workflow
andt2smap_workflow
) as well as private functions such as_main
and_get_parser
for the argument parsing and CLI.setup.py
to call the_main
functions in the workflows for the CLI.