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

test for new error in IVIM #1683

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@ShreyasFadnavis
Copy link
Contributor

ShreyasFadnavis commented Dec 7, 2018

PR for fix #1682

N = len(bvals_b0t)
bvecs = generate_bvecs(N)
gtab = gradient_table(bvals_b0t, bvecs.T)
assert_raises(ValueError, IvimModel, gtab)

This comment has been minimized.

@arokem

arokem Dec 7, 2018

Member

Are you sure this is throwing the new error? I suspect it might be throwing this one first: https://github.com/nipy/dipy/blob/master/dipy/reconst/ivim.py#L216

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Dec 7, 2018

Contributor

@arokem I think you are right! I did not realize this.. Will refactor and update ASAP :)

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Dec 7, 2018

Contributor

@arokem Checked the error and also added an assert to make sure that its the correct error! Does this work?

This comment has been minimized.

@arokem

arokem Dec 7, 2018

Member

Good stuff! +1 for the merge from me, if the CI comes back green.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 7, 2018

Codecov Report

Merging #1683 into master will increase coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1683      +/-   ##
==========================================
+ Coverage    84.1%   84.16%   +0.05%     
==========================================
  Files         113      113              
  Lines       13508    13508              
  Branches     2125     2125              
==========================================
+ Hits        11361    11369       +8     
+ Misses       1650     1642       -8     
  Partials      497      497
Impacted Files Coverage Δ
dipy/reconst/ivim.py 80.58% <ø> (+5.29%) ⬆️
dipy/data/fetcher.py 33.24% <0%> (ø) ⬆️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 7, 2018

Can you update the examples too? (Doc/example/reconst_ivim.py). It's failing with the new error. You can use this as a example for your test

@ShreyasFadnavis

This comment has been minimized.

Copy link
Contributor

ShreyasFadnavis commented Dec 7, 2018

Can you update the examples too? (Doc/example/reconst_ivim.py). It's failing with the new error. You can use this as a example for your test

@skoudoro.. Thank you for this! Will do this..

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 7, 2018

LGTM, waiting for the CI before merging it.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 7, 2018

CI's green and happy, merging!

@skoudoro skoudoro merged commit 62d30d5 into nipy:master Dec 7, 2018

3 of 4 checks passed

codecov/patch 0% of diff hit (target 84.1%)
Details
codecov/project 84.16% (+0.05%) compared to c67427b
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