Skip to content

Do not open nifti files with mmap if numpy < 1.12.0 #1796

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 31 commits into from
Feb 16, 2017

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Feb 2, 2017

Fixes #1795

@codecov-io
Copy link

codecov-io commented Feb 3, 2017

Codecov Report

Merging #1796 into master will increase coverage by 0.02%.
The diff coverage is 43.55%.

@@            Coverage Diff             @@
##           master    #1796      +/-   ##
==========================================
+ Coverage   69.82%   69.84%   +0.02%     
==========================================
  Files        1047     1047              
  Lines       52133    52161      +28     
  Branches     7669     7669              
==========================================
+ Hits        36403    36433      +30     
+ Misses      14142    14140       -2     
  Partials     1588     1588
Flag Coverage Δ
#unittests 69.84% <43.55%> (+0.02%)
Impacted Files Coverage Δ
nipype/workflows/fmri/fsl/preprocess.py 72.77% <ø> (-0.42%)
nipype/workflows/rsfmri/fsl/resting.py 85.48% <ø> (-1.41%)
nipype/utils/init.py 100% <100%> (ø)
nipype/algorithms/tests/test_errormap.py 100% <100%> (ø)
nipype/algorithms/tests/test_normalize_tpms.py 100% <100%> (ø)
nipype/utils/config.py 59.57% <100%> (+1.33%)
nipype/algorithms/tests/test_splitmerge.py 100% <100%> (ø)
nipype/workflows/dmri/dipy/denoise.py 22.72% <20%> (+1.18%)
nipype/workflows/misc/utils.py 17.54% <20%> (+1.47%)
nipype/algorithms/rapidart.py 34.69% <20%> (+0.19%)
... and 20 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 f5ed4d6...20e10f9. Read the comment docs.

.travis.yml Outdated
# Add install of vtk and mayavi to test mesh (disabled): conda install -y vtk mayavi &&
- function inst {
- function conda_inst {
export CONDA_HOME=/home/travis/anaconda &&
Copy link
Member

Choose a reason for hiding this comment

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

@oesteban - sorry if this was somehow my fault - but why did we switch to anaconda? miniconda gives us everything we need right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@satra I was just checking if switching to anaconda made any difference as regards the segmentation fault, but I'm arriving to the conclusion that miniconda is much better for travis. So there's no point on switching.

@oesteban
Copy link
Contributor Author

Ok, all-green lights on travis. It seems to me that conda has changed the way python should be downgraded. I was experimenting problems with python 3.4 not being downgraded. Now that should be fixed.

I have also split the install commands, to make it more granular: one for the apt stuff and a second for miniconda (still miniconda).

Other than that, this PR should get us away the casting problems while we can't enforce numpy>=1.12

@satra
Copy link
Member

satra commented Feb 11, 2017

@oesteban - quick question - would this affect all routines where we use nibabel? if so should we do this globally? and is this something nibabel is handling automatically now? i.e. if we change version of nibabel to current master this would be ok?

the reason i ask is that there is going to be a release of nibabel quite soon.

@oesteban
Copy link
Contributor Author

Hi @satra, at some point I had trouble with the version pinning of nibabel (https://travis-ci.org/nipy/nipype/jobs/198184510) and removed it. I'm to check if this is happening now.

However, I'd try not to set versions of packages unless we are truly sure that we need specific features that were not implemented in old versions.

Finally, we need to define more specific rules for maintaining dependencies (please see nipreps/niworkflows#117 (comment))

As regards deployment, the version specification for nibabel is still there (https://github.com/nipy/nipype/blob/master/nipype/info.py#L105). This means that for nipype installed through pypi, it will request nibabel>=2.0.1.

So the changes we make to the requirements.txt only affect development environments or special. This means that the operator should know what they are doing. Therefore, unless there are very strong reasons to enforce a package version (like traits>=4.5), in the requirements.txt we should not specify that (or, maybe better, we should try to have an empty or almost empty requirements.txt).

If you want to pin a package to the master branch of a repo, the requirements.txt is the place to do it. You can also try for the released pypi distribution, but so far, I've seen it working out only when no pypi package is available and there is no other option that the link_dependencies (more on this in the comment linked before).

For peace of mind reasons, I will reset the nibabel version on the requirements.txt until we know better what to do.

@oesteban
Copy link
Contributor Author

this is ready to merge :)

@satra
Copy link
Member

satra commented Feb 15, 2017

happy to merge this, but should we open an issue that does this for any place where nibabel load is used? it's used in most of the routines in algorithms.

@oesteban
Copy link
Contributor Author

oesteban commented Feb 15, 2017

Ok, wait a minute :) - using a regex to replace all easy appearances. Those more complicated (i.e. without the package alias nb. before) for another PR.

@oesteban
Copy link
Contributor Author

@satra there you go, all (I think none escaped the revision) the nibabel.load have now the mmap argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants