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

Flow csd sh order #1669

Merged
merged 4 commits into from Nov 27, 2018

Conversation

Projects
None yet
3 participants
@arokem
Copy link
Member

arokem commented Nov 26, 2018

In using the CSD workflow, I discovered that changing the sh_order input parameter doesn't seem to do anything. Tracking this down in the code, I see that it was hard-coded as sh_order = 8. This PR changes that and also adds some testing around that. It also more flexibly checks the inputs, so that we are not making assumptions about the input data.

# Test that the number of coefficients is what you would expect
# given the order of the sh basis:
assert_equal(shm_data.shape[-1],
sph_harm_ind_list(sh_order)[0].shape[0])

This comment has been minimized.

@arokem

arokem Nov 26, 2018

Member

This is where I test that the shape of the output conforms with the input sh_order provided.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@a7bb518). Click here to learn what that means.
The diff coverage is 96.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1669   +/-   ##
=========================================
  Coverage          ?   88.49%           
=========================================
  Files             ?      233           
  Lines             ?    29283           
  Branches          ?     3223           
=========================================
  Hits              ?    25914           
  Misses            ?     2699           
  Partials          ?      670
Impacted Files Coverage Δ
dipy/workflows/reconst.py 82.1% <50%> (ø)
dipy/workflows/tests/test_reconst_csa_csd.py 95.23% <98.11%> (ø)

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 a7bb518...0c884a6. Read the comment docs.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 27, 2018

LGTM, Thanks for this fix @arokem! I will merge it this evening

@skoudoro skoudoro merged commit 72f9516 into nipy:master Nov 27, 2018

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
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