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

Upgrade nibabel minimum version #1597

Merged
merged 3 commits into from Jul 31, 2018

Conversation

Projects
None yet
5 participants
@skoudoro
Copy link
Member

skoudoro commented Jul 25, 2018

Due to a memory issue on Streamlines, this is a proposition to upgrade the Nibabel minimal version.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jul 25, 2018

Hello @skoudoro, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 31, 2018 at 00:58 Hours UTC
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 25, 2018

In general +1 from me!

Could you point out relevant issues/PRs on nibabel that clarify what the memory issues are?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 25, 2018

yes, this is PR nipy/nibabel#597

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 25, 2018

BTW, Numpy released 2 days ago... and scipy did not release yet the fix #1565 so I think, all our PRs will failed from now until the scipy release. What should we do ?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 25, 2018

Can we patch in the scipy fix into our source tree for now?

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 25, 2018

I do not think so or I do not know how

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 25, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 25, 2018

Or are you referring to the numpy error (The one resulting in ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all())

that one was a cython error which has been corrected (#1579)

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 25, 2018

OK - so patching in our own version of fftpack (with the fixes from https://github.com/scipy/scipy/pull/8879/files) should do the trick.

@skoudoro skoudoro force-pushed the skoudoro:minimum-version branch from 15fa48d to 8babfcd Jul 30, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 30, 2018

Codecov Report

Merging #1597 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
+ Coverage   87.33%   87.34%   +<.01%     
==========================================
  Files         246      246              
  Lines       31814    31814              
  Branches     3451     3451              
==========================================
+ Hits        27786    27787       +1     
  Misses       3205     3205              
+ Partials      823      822       -1
Impacted Files Coverage Δ
dipy/info.py 100% <100%> (ø) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 b368e53...02ea61d. Read the comment docs.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 31, 2018

Does anyone else have any input on this one?

@matthew-brett : any reason you can think of not to update the dependencies to nibabel 2.3.0?

@matthew-brett

This comment has been minimized.

Copy link
Member

matthew-brett commented Jul 31, 2018

No reason I can think of.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jul 31, 2018

Off we go, then!

@arokem arokem merged commit 405ab40 into nipy:master Jul 31, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.33%)
Details
codecov/project 87.34% (+<.01%) compared to b368e53
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:minimum-version branch Jul 31, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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