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

ENH: BrainSuite updates: Add interfaces for additional command line tools #1554

Merged
merged 14 commits into from Sep 10, 2016

Conversation

jason-wg
Copy link
Contributor

@jason-wg jason-wg commented Aug 1, 2016

I have updated the BrainSuite Nipype interface to include classes for our BDP, SVReg and ThicknessPVC command line tools.

@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.05%) to 72.374% when pulling 56d5058 on jason-wg:master into 3ed411b on nipy:master.

@jason-wg
Copy link
Contributor Author

@satra @chrisfilo
Can you please take a look at my code updates, or refer this PR to someone who is available to review my code?

Thanks!

@satra
Copy link
Member

satra commented Aug 30, 2016

@jason-wg - quick observation - could you please try to get your code to be as close to PEP8 compliant as possible? as an example see: #1593

…online checker, removed all warnings, except for E501 Line Too Long
@jason-wg
Copy link
Contributor Author

@satra
I've made formatting fixes for PEP8 compliance, except that many lines run over 80 characters.
Does this look fine to you?

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage decreased (-0.02%) to 72.302% when pulling 04bb78c on jason-wg:master into 3ed411b on nipy:master.

@satra
Copy link
Member

satra commented Sep 8, 2016

@jason-wg - could you please merge with current master, fix any conflicts and push?

…n regarding encoding comment and imports for Python 3 compatability.
@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage decreased (-0.03%) to 72.237% when pulling 6a1e80b on jason-wg:master into 0f9a620 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 72.206% when pulling 6a1e80b on jason-wg:master into 0f9a620 on nipy:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage decreased (-0.03%) to 72.237% when pulling 15e51c3 on jason-wg:master into 0f9a620 on nipy:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 72.237% when pulling 15e51c3 on jason-wg:master into 0f9a620 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 72.237% when pulling 15e51c3 on jason-wg:master into 0f9a620 on nipy:master.

@jason-wg
Copy link
Contributor Author

jason-wg commented Sep 9, 2016

@satra

A note regarding the failure that we see in the Travis CI build:
Per the log, a doctest fails: Doctest: nipype.interfaces.brainsuite.brainsuite.BDP ... FAIL
When I run the doctest to take a closer look, the failure message is: TraitError: Each element of the 'BVecBValPair' trait of a BDPInputSpec instance must be a value of type 'str', but a value of u'/directory/subdir/prefix.dwi.bvec' <type 'unicode'> was specified.

For reference, the line containing the failing doctest is here
bdp.inputs.BVecBValPair is of type traits.List, requiring string inputs.
Based on the code in the doctest, I expect that both the items in the List are in fact strings.

I have tried to identify the cause of this failure, and I have found that when I take out the import of unicode_literals from this line that was recently added to my code in 632c1f1 , then the doctest runs fine.

When I add doctests for any other traits.List fields, I get the same error, ... instance must be a value of type 'str', but a value of ... <type 'unicode'> was specified

I'm curious about why this error is appearing, so please do let me know if you have any insights on this.

As for my pull request, what do you suggest I do for my code?

@satra
Copy link
Member

satra commented Sep 10, 2016

@jason-wg - you will need to: from ..traits_extension import Str

all strings are now unicode in nipype.

@coveralls
Copy link

coveralls commented Sep 10, 2016

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

@coveralls
Copy link

coveralls commented Sep 10, 2016

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 72.315% when pulling 7171c7d on jason-wg:master into 10f28fe on nipy:master.

@jason-wg
Copy link
Contributor Author

@satra
Thanks for letting me know. I've added the import you suggested, and that resolved the problem I was seeing.
I'm curious, what was the motivation behind making all strings unicode in Nipype?

Regarding this pull request, does this look good to you?

@satra
Copy link
Member

satra commented Sep 10, 2016

@jason-wg - to support py3 where all strings are unicode.

@satra satra merged commit bd3a7ef into nipy:master Sep 10, 2016
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.

None yet

3 participants