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

NF: Add is_hemispherical test #1850

Merged
merged 4 commits into from Jun 20, 2019

Conversation

@richford
Copy link
Contributor

commented Jun 6, 2019

This PR adds the is_hemispherical() function to dipy.core.geometry and adds associated unit tests. We found this function usefull in dmriprep to see if bvecs were collected from all points on a sphere or only hemispherically. @arokem suggested that it might by useful in other contexts and that I should add it to DIPY.

@pep8speaks

This comment has been minimized.

Copy link

commented Jun 6, 2019

Hello @richford, Thank you for updating !

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

Comment last updated at 2019-06-20 16:39:17 UTC

@richford richford changed the title Add is_hemispherical test NF: Add is_hemispherical test Jun 6, 2019

@richford

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Not sure if/where this new function should be documented. Should it go in examples/gradients_spheres.py? That doesn't quite fit. But it seems too minor and distracting to go in examples/quick_start.py.

@arokem

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Definitely not in the quick-start. Adding some more of the geometry module into the gradients and spheres wouldn't be too bad. Or we could have a separate example for the geometry module. We could also do that on a follow-up PR, once this is merged. For the time being, this all looks good to me. Could you just please rebase on master?

@arokem

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Actually, maybe best to wait until #1855 is merged before rebasing, so that you don't get spurious CI errors.

@richford richford force-pushed the richford:hemi_test branch from 8b8a57e to 8a9d175 Jun 18, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 18, 2019

Codecov Report

Merging #1850 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1850      +/-   ##
==========================================
+ Coverage   84.31%   84.33%   +0.01%     
==========================================
  Files         117      117              
  Lines       14234    14252      +18     
  Branches     2251     2255       +4     
==========================================
+ Hits        12001    12019      +18     
  Misses       1716     1716              
  Partials      517      517
Impacted Files Coverage Δ
dipy/core/geometry.py 91.22% <100%> (+0.59%) ⬆️
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Should be fine to rebase now.

@richford

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Rebased on master. Not sure why some of the checks are still failing:

  • Travis seems okay and reflects the most recent run (about a day ago) but codecov isn't happy. The build tab shows the updated successful builds but the header at the top references the failed builds 13 days ago (before rebase). Not sure how to make codecov update to reference the new travis builds.
  • appveyor is also unhappy for the Python35-x64 environment but you've already captured this in #1724.
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Codecov is kinda useless.

@skoudoro : what do you think -- should we turn that off?

@arokem

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Does anyone have any comments here? I think this should be ready to go.

CI errors are unrelated and already documented in pending issues.

@skoudoro
Copy link
Member

left a comment

Overall, it looks good to me too. Just a minor comment below.
Concerning codecov, it would be good to understand what happen and improve it but this can be done on another PR.
Thank you @richford for this !

# Test on hemispherical input
xyz = random_uniform_on_sphere(n=100, coords='xyz')
xyz = xyz[xyz[:, 2] > 0]
assert is_hemispherical(xyz)[0]

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jun 20, 2019

Member

Assert_equal is more explicit, I think it will be better

This comment has been minimized.

Copy link
@richford

richford Jun 20, 2019

Author Contributor

Thanks. Fixed in 4952d02.


# Test on spherical input
xyz = random_uniform_on_sphere(n=100, coords='xyz')
assert not is_hemispherical(xyz)[0]

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jun 20, 2019

Member

Same comment as above here

This comment has been minimized.

Copy link
@richford

richford Jun 20, 2019

Author Contributor

Thanks. Fixed in 4952d02.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Thank you @richford.

When CI finish, You can go ahead and merge it @arokem! Thank you !

@arokem arokem merged commit f26a404 into nipy:master Jun 20, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 84.31%)
Details
codecov/project 84.33% (+0.01%) compared to 2d049ce
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

🎉 Thanks @richford!

@richford richford deleted the richford:hemi_test branch Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.