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

BF: Use np.npy_intp instead of assuming long for ArraySequence attributes #1227

Merged
merged 3 commits into from Apr 20, 2017

Conversation

Projects
None yet
5 participants
@MarcCote
Contributor

MarcCote commented Apr 19, 2017

A small patch addressing the issue @arokem raised in comment #1076 (comment)

@codecov-io

This comment has been minimized.

codecov-io commented Apr 19, 2017

Codecov Report

Merging #1227 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1227      +/-   ##
==========================================
+ Coverage   85.88%   86.07%   +0.18%     
==========================================
  Files         221      223       +2     
  Lines       27285    27764     +479     
  Branches     2785     2824      +39     
==========================================
+ Hits        23435    23897     +462     
- Misses       3165     3173       +8     
- Partials      685      694       +9
Impacted Files Coverage Δ
dipy/info.py 100% <100%> (ø) ⬆️
dipy/reconst/dki.py 96.82% <0%> (-0.82%) ⬇️
dipy/reconst/mapmri.py 90.25% <0%> (-0.22%) ⬇️
dipy/reconst/dti.py 96.5% <0%> (-0.01%) ⬇️
dipy/reconst/tests/test_dki.py 100% <0%> (ø) ⬆️
dipy/reconst/tests/test_dki_micro.py 100% <0%> (ø)
dipy/reconst/dki_micro.py 92.7% <0%> (ø)

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 568d14c...44014ea. Read the comment docs.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 19, 2017

I think you'll need to bump up the minimum Cython version from 0.18 to 0.25.1 - see requirements.txt, .travis.yml, dipy/info.py ...

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 19, 2017

Unrelated test failure for pre-release wheels addressed by #1229.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 19, 2017

I get the int / long testing error reliably when using cython 0.18, 0.21, 0.24, but not 0.25.1. All of these compile OK, the error only appears at run-time. But why not try Cython==0.21 as the minimum dependency in .travis.yml and see? I think you'll find you need Cython==0.25.1 as the minimum dependency.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 19, 2017

@matthew-brett yes, you are right about needing Cython >= 0.25.1.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 19, 2017

Don't forget dipy/info.py and .travis.yml for the Cython dependency.

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.2%) to 88.562% when pulling 44014ea on MarcCote:fix_arrseq_length into 568d14c on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Apr 20, 2017

@matthew-brett thanks. @arokem this PR is ready.

@arokem

This comment has been minimized.

Member

arokem commented Apr 20, 2017

LGTM. Unless someone has more comments, I can merge this tomorrow.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 20, 2017

Looks good to me too.

@arokem arokem merged commit 49bc9db into nipy:master Apr 20, 2017

4 checks passed

codecov/patch 100% of diff hit (target 85.88%)
Details
codecov/project 86.07% (+0.18%) compared to 568d14c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 88.562%
Details

@MarcCote MarcCote deleted the MarcCote:fix_arrseq_length branch Apr 20, 2017

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

Merge pull request nipy#1227 from MarcCote/fix_arrseq_length
BF: Use np.npy_intp instead of assuming long for ArraySequence attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment