Skip to content

FIX: adding ICA_AROMA to fsl __init__.py #2105

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
Jul 26, 2017
Merged

FIX: adding ICA_AROMA to fsl __init__.py #2105

merged 4 commits into from
Jul 26, 2017

Conversation

jdkent
Copy link
Contributor

@jdkent jdkent commented Jun 28, 2017

Fixes: being able to import ICA_AROMA like other fsl packages

Changes proposed in this pull request

  • adds ICA_AROMA to fsl/interfaces/__init__.py

@effigies
Copy link
Member

Unfortunately, this is changing the API, since fsl.interfaces.ICA_AROMA is the module, not the class, so changing this has the potential to induce errors for anybody using this.

If it's worth making the change, we should make the name of the class and the module different, along with importing the class into __init__.py. I'd suggest changing the file name to ica_aroma.py or just aroma.py, and I'm inclined to leave the class name the same.

@satra I'd appreciate your input on a potential API change.

@satra
Copy link
Member

satra commented Jul 24, 2017

I'd suggest changing the file name to ica_aroma.py or just aroma.py, and I'm inclined to leave the class name the same.

i like that from .aroma import ICA_AROMA

let's make the file name change and leave the class intact.

@codecov-io
Copy link

Codecov Report

Merging #2105 into master will increase coverage by 3.8%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2105     +/-   ##
=========================================
+ Coverage   48.56%   52.36%   +3.8%     
=========================================
  Files         116      119      +3     
  Lines       23598    23770    +172     
=========================================
+ Hits        11461    12448    +987     
+ Misses      12137    11322    -815
Flag Coverage Δ
#smoketests 52.36% <100%> (+3.8%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/fsl/__init__.py 100% <100%> (ø) ⬆️
examples/fmri_fsl_feeds.py 96.61% <0%> (ø)
nipype/interfaces/fsl/ICA_AROMA.py 68.42% <0%> (ø)
examples/fmri_fsl_reuse.py 75.67% <0%> (ø)
nipype/pipeline/engine/utils.py 56.71% <0%> (+0.27%) ⬆️
nipype/interfaces/io.py 35.57% <0%> (+0.64%) ⬆️
nipype/utils/filemanip.py 73.82% <0%> (+1%) ⬆️
nipype/pipeline/engine/nodes.py 67.26% <0%> (+1.52%) ⬆️
nipype/pipeline/engine/workflows.py 58.23% <0%> (+2.03%) ⬆️
nipype/algorithms/modelgen.py 46.04% <0%> (+3.72%) ⬆️
... and 10 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 af2b7aa...dad4afe. Read the comment docs.

@effigies effigies merged commit f85cf0d into nipy:master Jul 26, 2017
@satra satra added this to the 0.14.0 milestone Oct 20, 2017
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