Skip to content
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

Move the tests from test_refine_rb to test_bundles. #1891

Merged
merged 3 commits into from Jul 11, 2019

Conversation

@arokem
Copy link
Member

commented Jul 8, 2019

This addresses #1889 and also fixes up a few things in dipy.segment.bundles:

  1. Removed an undocumented two-liner in favor of inline code.
  2. Addresses an issue where empty indices throw an error.

Testing in this module is still pretty spotty, as far as I can tell.

arokem added 2 commits Jul 8, 2019
Move the tests from test_refine_rb to test_bundles.
Also, removes an undocumented two-liner and fix a bug that
happens when an indexing array is empty.
@pep8speaks

This comment has been minimized.

Copy link

commented Jul 8, 2019

Hello @arokem, Thank you for updating !

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

Comment last updated at 2019-07-08 19:31:37 UTC
@codecov-io

This comment has been minimized.

Copy link

commented Jul 9, 2019

Codecov Report

Merging #1891 into master will increase coverage by 0.4%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1891     +/-   ##
=========================================
+ Coverage   84.91%   85.31%   +0.4%     
=========================================
  Files         118      118             
  Lines       14298    14221     -77     
  Branches     2248     2236     -12     
=========================================
- Hits        12141    12133      -8     
+ Misses       1645     1577     -68     
+ Partials      512      511      -1
Impacted Files Coverage Δ
dipy/segment/bundles.py 91.37% <50%> (-0.11%) ⬇️
dipy/tracking/streamline.py 92.95% <0%> (+22.22%) ⬆️
@skoudoro
Copy link
Member

left a comment

Thank you@arokem for this. I have just a small comment, but it seems ready to be merged

pruned_streamlines = transf_streamlines[np.array(pruned_indices)]
idx = np.array(pruned_indices)
if len(idx) == 0:
print(' You have removed all streamlines')

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 11, 2019

Member

logging instead of print ? or it should be merged before #1862

This comment has been minimized.

Copy link
@arokem

arokem Jul 11, 2019

Author Member

If this is good, let's merge this one first, and then I can rebase #1862 and fix this up.

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 11, 2019

Member

sounds like a plan! merging

@skoudoro skoudoro merged commit cbc9515 into nipy:master Jul 11, 2019

4 of 5 checks passed

codecov/patch 50% of diff hit (target 84.91%)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/project 85.31% (+0.4%) compared to 33df2b5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.