[ENH] Drop all ICA/MELODIC work. Expect individual files. #48
Conversation
|
Thanks! The only other thing I think we need before merging is @oesteban's thoughts. Unfortunately, I can't request a review from him, so tagging will have to suffice. |
Since this PR is delaying #46 , why don't we merge if @oesteban has no comments by this Friday? If he has any comments, we could also address them after this PR is merged. |
There was a problem hiding this comment.
I just have a nit-pick about assertions. My review is pretty shallow, and honestly I haven't tried to follow the logic through. However, the fact that this PR is simplifying the codebase a lot (514 delete vs. 142 adds) is very much needed. I think this is the right move to escape away from the cohesiveness of the original AROMA implementation with MELODIC.
aroma/aroma.py
Outdated
| motion_params = utils.load_motpars(mc, source=mc_source) # T x 6 | ||
| mixing = np.loadtxt(mixing) # T x C | ||
| component_maps = nib.load(component_maps) # X x Y x Z x C | ||
| assert mixing.shape[1] == component_maps.shape[3], f"{mixing.shape}, {component_maps.shape}" |
There was a problem hiding this comment.
These assertions within the executed code (i.e., not a test) should be better raised as python errors. The reason is that an error is much more informative of what happened (e.g., a ValueError in this particular line because the mixing and component dimensions do not match, or a FileNotFoundError are way more clear than assertion errors and a description that might be more or less informative)
There was a problem hiding this comment.
That's fair. I replaced the assertions outside of tests in 9633a32.
|
@eurunuela CI is passing, so do you mind if I merge? |
Go ahead! |
Closes #27. I think this will bring us a lot closer to an MVP for fMRIPrep integration.
Changes proposed in this pull request: