Skip to content

ENH: ICA_AROMA: make current working directory default output directory #2056

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

Merged
merged 4 commits into from
Jun 3, 2017
Merged

ENH: ICA_AROMA: make current working directory default output directory #2056

merged 4 commits into from
Jun 3, 2017

Conversation

jdkent
Copy link
Contributor

@jdkent jdkent commented May 27, 2017

Changes proposed in this pull request

  • Make the default output directory the current working directory so you don't have to specify an out_dir, which I believe makes ICA_AROMA more Node friendly.

@jdkent
Copy link
Contributor Author

jdkent commented May 27, 2017

I was a little hasty getting this up. Please ignore until I fix it. Fixed.

@codecov-io
Copy link

codecov-io commented May 27, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@9d63943). Click here to learn what that means.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2056   +/-   ##
=========================================
  Coverage          ?   72.16%           
=========================================
  Files             ?     1137           
  Lines             ?    57194           
  Branches          ?     8194           
=========================================
  Hits              ?    41276           
  Misses            ?    14629           
  Partials          ?     1289
Flag Coverage Δ
#smoketests 72.16% <25%> (?)
#unittests 69.78% <25%> (?)
Impacted Files Coverage Δ
nipype/interfaces/fsl/tests/test_auto_ICA_AROMA.py 85.71% <ø> (ø)
nipype/interfaces/fsl/ICA_AROMA.py 68.42% <25%> (ø)

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 9d63943...48fcf86. Read the comment docs.

@satra
Copy link
Member

satra commented Jun 3, 2017

@jdkent - could you please resolve conflicts with master? relative imports should always be used in nipype - we stopped being careful during review.

@jdkent
Copy link
Contributor Author

jdkent commented Jun 3, 2017

@satra: you've probably answered this a thousand times, but why use relative versus absolute imports? Not that I abide by PEP for everything, but PEP 328 wants to make absolute imports standard. Does it have to do with making sure we are importing the right packages if we have multiple versions of nipype installed? Sorry for badgering, but I am curious.

@effigies
Copy link
Member

effigies commented Jun 3, 2017

@jdkent I think you may be misreading PEP 328, which is about interpretation of import statements. Pre-PEP328, import x would look for x in the local package, i.e. all imports were assumed relative. PEP328 moved to implicit absolute imports and explicit relative imports.

But yes, using relative imports does ensure that we use a package-local version, rather than one found in the PYTHONPATH. (It also helps with including nipype as a submodule in other packages.)

Incidentally, @oesteban it looks like from __future__ import absolute_import was only necessary pre-2.6? Nipype doesn't support before 2.7, so that's probably why all the missing absolute_import requirements haven't had any effect.

@satra satra merged commit 6be8010 into nipy:master Jun 3, 2017
out_dir = os.path.abspath(self.inputs.out_dir)
outputs['out_dir'] = out_dir

if isdefined(self.inputs.out_dir):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need the isdefined module for this line to run, please let me know if I'm mistaken.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Please add isdefined to the from ..base import line.

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.

4 participants