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

Small fix in free water DTI model #1124

Merged
merged 2 commits into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@RafaelNH
Contributor

RafaelNH commented Sep 26, 2016

Procedure was crashing when negative S0 are present. In theory such values are implausible. However, in practice this values can be present in datasets! For example, I found this cases when processing the full CENIR dataset.

When such cases are detected all parameters are directly set to zero.

BF: Deal with negative S0
in teory we shouldn't have negative S0, however this can happen in partice. For example this case was found in the CENIR HCP like dataset
@arokem

This comment has been minimized.

Member

arokem commented Sep 26, 2016

Negative signal?! Must be some weird artifact of preprocessing. Was the error you got very hard to decipher? Otherwise I am thinking that maybe throwing an error might actually be the right thing to do.

@codecov-io

This comment has been minimized.

codecov-io commented Sep 26, 2016

Current coverage is 81.00% (diff: 100%)

Merging #1124 into master will increase coverage by 0.01%

@@             master      #1124   diff @@
==========================================
  Files           213        213          
  Lines         24032      24047    +15   
  Methods           0          0          
  Messages          0          0          
  Branches       2434       2434          
==========================================
+ Hits          19465      19480    +15   
  Misses         4063       4063          
  Partials        504        504          

Powered by Codecov. Last update d0bee8c...d66a0dd

@coveralls

This comment has been minimized.

coveralls commented Sep 26, 2016

Coverage Status

Coverage increased (+0.007%) to 83.092% when pulling 9c93e29 on RafaelNH:fwdti_fix into d0bee8c on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 26, 2016

Coverage Status

Coverage increased (+0.007%) to 83.092% when pulling 9c93e29 on RafaelNH:fwdti_fix into d0bee8c on nipy:master.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Sep 26, 2016

Hi @arokem! I know that some eddy currents artifact correction algorithms can produce this negative values - it might be the case here. In dti and dki models we don't have this problem because all intensities smaller that min_signal are replaced by min_signal. My first attempt to correct this problem here was to follow the same idea. If we do that, FA and MD values for these cases tend to zero however f values can tend to any value on its range [0-1]. The correction that I am doing here insures that f is also set to zero for these cases. The only way that users can manually correct this problem is indeed replacing negative or zero values with a min signal intensity, thus I am not sure if adding an error message is a good idea. I don't want that the processing of the fwdti model stops for a problem that dti and dki automatically deals. Also these problematic voxels are normally only present in regions of the skull or background. What do you think? Let me know your coding suggestions I will address them asap.

@arokem

This comment has been minimized.

Member

arokem commented Sep 26, 2016

All clear - thanks for the explanation. I'll go ahead and review.

@arokem

Just a couple of small comments. Overall, LGTM.

@@ -295,3 +295,18 @@ def test_md_regularization():
assert_array_almost_equal(fwefit.fa, FAref)
assert_array_almost_equal(fwefit.md, MDref)
assert_array_almost_equal(fwefit.f, GTF)


def test_zero_s0():

This comment has been minimized.

@arokem

arokem Sep 26, 2016

Member

Should this be test_negative_s0?

This comment has been minimized.

@RafaelNH

RafaelNH Sep 26, 2016

Contributor

Done =)

fwefit = fwdm.fit(S_conta)
assert_array_almost_equal(fwefit.fa, 0.0)
assert_array_almost_equal(fwefit.md, 0.0)
assert_array_almost_equal(fwefit.f, 0.0)

This comment has been minimized.

@arokem

arokem Sep 26, 2016

Member

Would you mind adding a multi-voxel case where one voxel has negative S0 and others do not, just to make sure that we don't get cross-talk (that is - that the parameters in other voxels are unaffected)?

This comment has been minimized.

@RafaelNH

RafaelNH Sep 26, 2016

Contributor

Done =)

@arokem

arokem approved these changes Sep 26, 2016

@coveralls

This comment has been minimized.

coveralls commented Sep 26, 2016

Coverage Status

Coverage increased (+0.01%) to 83.096% when pulling d66a0dd on RafaelNH:fwdti_fix into d0bee8c on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Sep 26, 2016

This LGTM. Considering this is a small fix, I will go ahead and merge this in a couple of days, unless someone voices opposition.

@arokem arokem merged commit 994d43f into nipy:master Sep 28, 2016

4 checks passed

codecov/patch 100% of diff hit (target 80.99%)
Details
codecov/project 81.00% (+0.01%) compared to d0bee8c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 83.096%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment