bug for datagrabber

[anbai106]

There are two other classes in the io.py file, they maybe have the same problem, if you think my change works, you can also change the other two classes.

[codecov-io]

Codecov Report

Merging #1915 into master will increase coverage by 0.05%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   67.43%   67.49%   +0.05%     
==========================================
  Files        1224     1225       +1     
  Lines       60187    60308     +121     
  Branches     8647     8649       +2     
==========================================
+ Hits        40590    40703     +113     
- Misses      18471    18489      +18     
+ Partials     1126     1116      -10

Flag	Coverage Δ
#smoketests	51.27% <60%> (+0.57%)	⬆️
#unittests	65.45% <20%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/io.py	50.46% <60%> (+0.03%)	⬆️
nipype/pipeline/plugins/multiproc.py	79.76% <0%> (-0.6%)	⬇️
examples/fmri_spm_nested.py	86.44% <0%> (ø)
nipype/interfaces/base/core.py	85.11% <0%> (+0.15%)	⬆️
nipype/pipeline/engine/utils.py	82.75% <0%> (+0.22%)	⬆️
nipype/utils/filemanip.py	75% <0%> (+0.96%)	⬆️
nipype/utils/config.py	66.29% <0%> (+1.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 166e06c...b8eda2d. Read the comment docs.

[satra]

@mgxd - could you please review this and make sure it functions as intended in the description for raise_on_empty?

[mgxd]

@anbai106 maybe we can add a new input field drop_blank_outputs that performs this check on top of raise_on_empty.

[mgxd]

@anbai106 the behavior you want would require a new input for datagrabber - something along the lines of

...
        if self.inputs.drop_blank_outputs:
            outputs[key] = [x for x in outputs[key] if x is not None]
        else:
            if any([val is None for val in outputs[key]]):
                outputs[key] = []
...

this behavior will potentially return lists of different sizes, which is dangerous when working with mapnodes / iterables across runs - thus, better to make a new input rather than build off of raise_on_empty

(linking to related issue #1783)

This is very close to merge!

[effigies]

Hi @anbai106, are you going to have time to finish this up, soon?

[anbai106]

@effigies sorry for the delay, not so familiar with git pull request, i hope now everything is OK (BTW, should I also add the new input field of drop_blank_outputs from @mgxd?? I did do that before the merge...).

[mgxd]

@anbai106 we're just missing two things:

could add a new input for DataGrabber here?

nipype/nipype/interfaces/io.py

Line 1039 in 166e06c

class DataGrabberInputSpec(DynamicTraitedSpec, BaseInterfaceInputSpec):

and make the changes from the earlier comment?

If you don't find time, we could make the changes for you, if that would be easier. Thanks!

[anbai106]

@mgxd , I am doing it now, if I make it wrong this time, you will be in charge of it, haha

[anbai106]

@mgxd I corrected like you proposed and merge my branch into the master branch, can you check if everything is ok?

[mgxd]

@anbai106 you'll have to git push your branch for us to see the changes

[effigies]

Hi @anbai106, I think you merged into your own master branch, which isn't going to show up here. Try the following:

git remote add upstream git@github.com:nipy/nipype.git
git fetch upstream

This will make the official master branch available to you as upstream/master. Then we need to merge your current changes into the anbai branch, which is connected to this pull request.

git checkout anbai
git merge master

Finally, I would recommend merging the current master, just to update the tests, then push:

git merge upstream/master
git push

[anbai106]

@effigies finally, I think it is done :)
Thanks for your help

[effigies]

@anbai106 For some reason that latest version had some commit issues in it. I rebased your PR against the current master to remove the noise. Can you please check the diff and verify that it looks the way you intended?

[effigies]

@mgxd I think this is ready, unless you need tests.

[mgxd]

LGTM, thanks @anbai106 @effigies