Skip to content
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

FIX: correct bug in _concatenate_info function #2857

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hstojic
Copy link
Contributor

@hstojic hstojic commented Jan 18, 2019

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)])

Using concatenate_runs = True results in an error as regressor names are missing for session dummy variables, this one-line corrects it.
effigies
effigies previously approved these changes Jan 20, 2019
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible. I think it could be written slightly more cleanly. And can you verify that this produces the results you expect?

And do you want to add yourself to the .zenodo.json file?

@@ -567,6 +567,8 @@ def _concatenate_info(self, infolist):
infoout.regressors.insert(
len(infoout.regressors),
onelist.tolist()[0])
# insert session regressor name
infoout.regressor_names.extend(['run' + str(i + 1)])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
infoout.regressor_names.extend(['run' + str(i + 1)])
infoout.regressor_names.append('run%d' % (i + 1))

@effigies effigies added this to the 1.1.8 milestone Jan 20, 2019
@hstojic
Copy link
Contributor Author

hstojic commented Jan 20, 2019

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 added a commit to hstojic/nipype that referenced this pull request Jan 20, 2019
Related to the fix in nipy#2857
@hstojic hstojic mentioned this pull request Jan 20, 2019
@effigies
Copy link
Member

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
Copy link
Member

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

@effigies effigies dismissed their stale review January 20, 2019 22:13

Failing tests

@hstojic
Copy link
Contributor Author

hstojic commented Jan 21, 2019

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.

corrected and error in _concatenate_info - when regressor and regressor_names are not defined, the fix fails. Now the regressor_names are created if they were not defined by the user.
@codecov-io
Copy link

codecov-io commented Jan 21, 2019

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.81%. Comparing base (e07301d) to head (5a82527).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2857      +/-   ##
==========================================
- Coverage   70.81%   70.81%   -0.01%     
==========================================
  Files        1277     1277              
  Lines       59153    59155       +2     
  Branches     8590     8590              
==========================================
  Hits        41889    41889              
- Misses      16119    16120       +1     
- Partials     1145     1146       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@satra
Copy link
Member

satra commented Jan 21, 2019

@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.

@effigies effigies mentioned this pull request Jan 25, 2019
16 tasks
@effigies effigies modified the milestones: 1.1.8, future Jan 27, 2019
@hstojic
Copy link
Contributor Author

hstojic commented Jan 29, 2019

@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

@effigies
Copy link
Member

No worries. When you get some time.

@effigies effigies modified the milestones: future, 1.1.9 Jan 29, 2019
@effigies
Copy link
Member

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 effigies modified the milestones: 1.1.9, 1.2.0 Feb 25, 2019
@mgxd mgxd modified the milestones: 1.2.0, future Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants