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

Fix calculation of highest order for a sh basis set #1589

Merged
merged 3 commits into from Jul 20, 2018

Conversation

Projects
None yet
4 participants
@arokem
Copy link
Member

arokem commented Jul 18, 2018

Starting to think about addressing gh-1588.

arokem added some commits Jul 18, 2018

Check that the output is an even whole number.
If it isn't, that means the input was not a valid number of coefficients.
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jul 18, 2018

Hello @arokem, Thank you for updating !

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

Comment last updated on July 18, 2018 at 16:18 Hours UTC
@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jul 18, 2018

LGTM!

Thanks @arokem .

I'll wait until Travis finishes, and if everything is good I'll go ahead with this since it's a really well contained change. Except if you think someone else should also review it?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 18, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
+ Coverage   87.32%   87.34%   +0.01%     
==========================================
  Files         246      246              
  Lines       31936    31938       +2     
  Branches     3471     3472       +1     
==========================================
+ Hits        27889    27896       +7     
+ Misses       3220     3215       -5     
  Partials      827      827
Impacted Files Coverage Δ
dipy/reconst/shm.py 86.68% <100%> (+0.04%) ⬆️
dipy/reconst/tests/test_shm.py 98.9% <100%> (ø) ⬆️
dipy/reconst/mapmri.py 90.93% <0%> (+0.64%) ⬆️

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 9994c2a...2f0d342. Read the comment docs.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jul 20, 2018

Since there was no additional feedback and this was really small, I'll go ahead and merge.

Thanks @arokem

@jchoude jchoude merged commit e24ad73 into nipy:master Jul 20, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.32%)
Details
codecov/project 87.34% (+0.01%) compared to 9994c2a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request nipy#1589 from arokem/fix_max_sh
Fix calculation of highest order for a sh basis set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment