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

RF: Remove patch for older numpy ravel_multi_index. #1300

Merged
merged 1 commit into from Nov 25, 2017

Conversation

Projects
None yet
5 participants
@arokem
Member

arokem commented Jul 13, 2017

Attempting to address #1299.

It looks like the test that is failing there is related to a patch of numpy's ravel_multi_index for older versions of numpy. I am putting in this PR to see whether we can do without that patch altogether at this point.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-0.0008%) to 85.434% when pulling d4c9e9c on arokem:gh1299 into 736cbb5 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-0.0008%) to 85.434% when pulling d4c9e9c on arokem:gh1299 into 736cbb5 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 13, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1300      +/-   ##
==========================================
+ Coverage   87.13%   87.13%   +<.01%     
==========================================
  Files         228      228              
  Lines       28800    28764      -36     
  Branches     3093     3090       -3     
==========================================
- Hits        25094    25064      -30     
+ Misses       3003     2998       -5     
+ Partials      703      702       -1
Impacted Files Coverage Δ
dipy/tracking/tests/test_utils.py 99.28% <ø> (+0.41%) ⬆️
dipy/tracking/utils.py 87.57% <100%> (+0.68%) ⬆️

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 736cbb5...d4c9e9c. Read the comment docs.

@arokem

This comment has been minimized.

Member

arokem commented Jul 13, 2017

Yep. Looks like we don't need this patch anymore. Might already be fixed on our minimal numpy requirement?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 24, 2017

Do we need to remove the test completely?

@arokem

This comment has been minimized.

Member

arokem commented Jul 24, 2017

Yes. I think that if we remove the implementation, we should also remove the test.

The use of numpy's ravel_multi_index is tested through it's downstream use in ndbincount.

@yarikoptic

This comment has been minimized.

Member

yarikoptic commented Nov 23, 2017

Could decision be made on this PR since it is a blocker for #1299 !

@arokem

This comment has been minimized.

Member

arokem commented Nov 24, 2017

Hey @Garyfallidis : any more thoughts about this one?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 24, 2017

Sure, no issue on my side @arokem ! Go ahead! :)

@arokem

This comment has been minimized.

Member

arokem commented Nov 25, 2017

I will take that as approval to go ahead and merge this PR.

@arokem arokem merged commit f3b517b into nipy:master Nov 25, 2017

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.0008%) to 85.434%
Details
codecov/patch 100% of diff hit (target 87.13%)
Details
codecov/project 87.13% (+<.01%) compared to 736cbb5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Member

arokem commented Nov 25, 2017

Resolves #1299

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

Merge pull request nipy#1300 from arokem/gh1299
RF: Remove patch for older numpy ravel_multi_index.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment