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

test for new error in IVIM #1683

Merged
merged 2 commits into from Dec 7, 2018
Merged

test for new error in IVIM #1683

merged 2 commits into from Dec 7, 2018

Conversation

ShreyasFadnavis
Copy link
Member

@ShreyasFadnavis 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@codecov-io
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
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
Copy link
Member Author

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
Copy link
Member

skoudoro commented Dec 7, 2018

LGTM, waiting for the CI before merging it.

@skoudoro
Copy link
Member

skoudoro commented Dec 7, 2018

CI's green and happy, merging!

@skoudoro skoudoro merged commit 62d30d5 into dipy:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for IVIM for new Error
4 participants