nipype icon indicating copy to clipboard operation
nipype copied to clipboard

FIX: correct bug in _concatenate_info function

Open hstojic opened this issue 6 years ago • 9 comments

This is related to issue #2844 that I raised, more details is provided there.

In summary, using concatenate_runs = True results in an error as regressor names are missing for session dummy variables, this one-line corrects it by adding a session regressor name to the regressor_names variable..

If my interpretation is correct, its an easy fix with a single line I think: infoout.regressor_names.extend(['run' + str(i + 1)])

hstojic avatar Jan 18 '19 18:01 hstojic

Yes, this produces the results I expect, I already use it in my local code.

I will add myself to the zenodo file, thanks!

hstojic avatar Jan 20 '19 20:01 hstojic

Just noticed the failing tests. Looks like infoout.regressor_names doesn't exist to extend. If you look at lines 563-564, you need to create the list first. I'm a little curious why it works for you, then...

I'll try to have a deeper look tomorrow.

effigies avatar Jan 20 '19 22:01 effigies

Also, can you go ahead and merge/cherry-pick 4158583 into this PR instead of having a separate one?

effigies avatar Jan 20 '19 22:01 effigies

Ok, silly mistake, regressors is an optional argument and so is regressor_names then. In my case I had nuisance regressors and their names so it never complained. Tests for sure have cases without any nuisance covariate. should be Ok by just adding creating the list few lines above.

hstojic avatar Jan 21 '19 15:01 hstojic

Codecov Report

Merging #2857 into master will decrease coverage by 0.04%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2857      +/-   ##
==========================================
- Coverage   67.46%   67.42%   -0.05%     
==========================================
  Files         341      341              
  Lines       43392    43671     +279     
  Branches     5383     5492     +109     
==========================================
+ Hits        29276    29445     +169     
- Misses      13419    13499      +80     
- Partials      697      727      +30
Flag Coverage Δ
#smoketests 50.22% <0%> (-0.32%) :arrow_down:
#unittests 64.88% <100%> (-0.01%) :arrow_down:
Impacted Files Coverage Δ
nipype/algorithms/modelgen.py 65.52% <100%> (-1.92%) :arrow_down:
nipype/pipeline/plugins/legacymultiproc.py 61.5% <0%> (-4.5%) :arrow_down:
nipype/workflows/fmri/spm/preprocess.py 70.4% <0%> (-2.05%) :arrow_down:
nipype/utils/provenance.py 82.8% <0%> (-1.28%) :arrow_down:
nipype/pipeline/engine/nodes.py 84.19% <0%> (-0.34%) :arrow_down:
nipype/interfaces/dynamic_slicer.py 17.47% <0%> (ø) :arrow_up:
nipype/interfaces/nipy/preprocess.py 45.79% <0%> (ø) :arrow_up:
nipype/interfaces/io.py 53.99% <0%> (ø) :arrow_up:
nipype/interfaces/freesurfer/preprocess.py 66.11% <0%> (ø) :arrow_up:
... and 2 more

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 ebbd062...964fc51. Read the comment docs.

codecov-io avatar Jan 21 '19 15:01 codecov-io

@hstojic and @effigies - since this creates a patch without a broken test, it would be great to have a broken test as well. i just asked @hstojic to add that in the original issue.

satra avatar Jan 21 '19 16:01 satra

@satra and @effigies I have been a bit tight on time these few days to add a test and resolve the issue before the new release, it will have to wait for the new one, I'll try to do it soonish

hstojic avatar Jan 29 '19 07:01 hstojic

No worries. When you get some time.

effigies avatar Jan 29 '19 14:01 effigies

Hi @hstojic. Just a bump to let you know we'll be aiming to have everything in for the 1.1.9 release by next Friday, Feb 22. Let us know if you need any help on this.

effigies avatar Feb 14 '19 15:02 effigies