[FIX] Define ANTSPATH for ants.BrainExtraction automatically #1986
Conversation
When it is not already defined. Also check if the interface failed for that reason nonetheless. Fixes nipy#1901
Codecov Report
@@ Coverage Diff @@
## master #1986 +/- ##
==========================================
- Coverage 72.29% 72.27% -0.03%
==========================================
Files 1089 1089
Lines 55583 55601 +18
Branches 8009 8013 +4
==========================================
+ Hits 40182 40183 +1
- Misses 14137 14164 +27
+ Partials 1264 1254 -10
Continue to review full report at Codecov.
|
| (self.cmd.split()[0], runtime.hostname)) | ||
|
|
||
| # Set the environment variable if found | ||
| runtime.environ.update({'ANTSPATH': os.path.dirname(cmd_path)}) |
There was a problem hiding this comment.
i believe ANTSPATH requires a trailing /. could you confirm this and make sure we adjust for it?
There was a problem hiding this comment.
Most of the scripts don't need the trailing /. Particularly, the antsBrainExtraction.sh doesn't need it.
But I can add it anyways if you want.
| runtime = super(BrainExtraction, self)._run_interface(runtime) | ||
|
|
||
| # Still, double-check if it didn't found N4 | ||
| if 'we cant find the N4 program' in runtime.stdout: |
There was a problem hiding this comment.
since the script can be in a different location than the N4 executable, can we not do this check before running in this particular case?
There was a problem hiding this comment.
Sorry I'm lost here. Which check would you remove?
There was a problem hiding this comment.
i meant to put the N4 check before calling _run_interface instead of checking the output.
There was a problem hiding this comment.
Judging from this: https://github.com/stnava/ANTs/blob/646459bf297a50cdf608ea7d39cb79b44906ad22/Scripts/antsBrainExtraction.sh#L5-L20, I think we just parse the output for "we cant find" and raise the message for the missing tool.
For next release we can decouple or, as @blakedewey suggested, replicate antsBrainExtraction in pure nipype.
|
in general, for next release, i would like to decouple these checks as requirements of interfaces and have a generic checker mechanism that spans local paths and container id/paths. |
|
Sounds good. The problem here is a bit different, since it is the definition of |
|
This looks good to me. It should cover the bases for when errors could occur. I agree that a more general solution in the future would be preferable, even recreating this script in Nipype would be a great addition as well. |
|
+1 to adding a pure-nipype workflow for antsBrainExtraction |
|
@satra I left a couple of questions, if you had a minute I can get this ready to merge :) |
Only when it is not already defined. Also check if the interface failed for that reason nonetheless.
Fixes #1901