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 to insure that fwDTI non-linear procedure does not crash #1137

Merged
merged 7 commits into from Nov 14, 2016

Conversation

Projects
None yet
4 participants
@RafaelNH
Contributor

RafaelNH commented Oct 31, 2016

Analogous to DTI non-linear code, I added some lines of code to insure that fwDTI does not crash when the non-linear procedure gives NaN.

@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2016

Could you please add a test? I assume you've encountered some data that raises this?

@coveralls

This comment has been minimized.

coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.006%) to 87.128% when pulling 58b10ab on RafaelNH:fix_fwdti_eigenvalues into 9bbb5a4 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Oct 31, 2016

Current coverage is 84.64% (diff: 100%)

Merging #1137 into master will increase coverage by 0.02%

@@             master      #1137   diff @@
==========================================
  Files           219        219          
  Lines         24884      24897    +13   
  Methods           0          0          
  Messages          0          0          
  Branches       2514       2514          
==========================================
+ Hits          21056      21073    +17   
+ Misses         3199       3195     -4   
  Partials        629        629          

Powered by Codecov. Last update d4f45f2...091c7cf

@RafaelNH RafaelNH force-pushed the RafaelNH:fix_fwdti_eigenvalues branch from 9a68aa1 to 017c59f Nov 1, 2016

@arokem

Great. I like the comprehensive approach. Thanks for the elegant solution to this problem!

Just a couple of really small changes proposed.

And a couple of questions:

  1. Can you get a LinAlgError even with an input that is not all nans? Might be good to add something like that as a test as well. The all-nans is a bit contrived, if it is possible otherwise.
  2. Does it make sense to set a reasonable default for tensor_alternative? Does it make sense to check whether this input really is Hermitian, before using it (maybe inside the except block?)?
@@ -1513,6 +1512,51 @@ def _nlls_jacobian_func(tensor, design_matrix, data, *arg, **kwargs):
return -pred[:, None] * design_matrix


def _decompose_tensor_nan(tensor, tensor_alternative, min_diffusivity=0):
""" Helper function that expandes the function decompose_tensor to deal

This comment has been minimized.

@arokem

arokem Nov 1, 2016

Member

"expandes" => "expands"

eigenvectors (Basser et al., 1994a). Some fit approaches can produce nan
tensor elements in background voxels (particularly non-linear approachs).
This function avoids the eigen decomposition errors of nan tensor elements
by replacing tinsor with nan elements by a given alternative tensor

This comment has been minimized.

@arokem

arokem Nov 1, 2016

Member

"tinsor" => "tensor"

@@ -706,3 +707,20 @@ def test_eig_from_lo_tri():

lo_tri = lower_triangular(dmfit.quadratic_form)
assert_array_almost_equal(dti.eig_from_lo_tri(lo_tri), dmfit.model_params)

This comment has been minimized.

@arokem

arokem Nov 1, 2016

Member

PEP8: one more line here (two lines between functions)

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Nov 1, 2016

Hi @arokem - actually finding an concrete signal to reproduce this problem is not that trivial. I encountered this problem when processing more than 600 subjects in my units cluster. The problem only occurred in background voxels of 9 subjects. I tried to create a test based on these signals however I noticed that the previous codes were processing these signals fine with the previous code version. After looking to the source of the issue in the cluster nodes, I notice that this has happening due to an overflow of the exponential function of non-linear diffusion model fits which were given NaN tensor elements. Since in my home computer I was not getting this problem, I assume that testing this issue from signals depend on the system that you are running dipy. Therefore, I decided to test the problem from tensors with NaN elements. For this, I created a helper function with the lines of code that were dealing with this problem for the standard dti non-linear fit. With this I am reducing duplicate code and testing previously non covered lines of dti.py - the test was added in test_dti.py.

@arokem

This comment has been minimized.

Member

arokem commented Nov 1, 2016

That all makes sense - thank you for expanding on that.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Nov 1, 2016

@arokem - just noticed your comments! I will carry on the work tomorrow (getting late here in Cambridge). I think my last comment covers your 1st point, right? Regarding point 2 - since _decompose_tensor_nan is only a helper function, I didn't want to increase complexity and eventually decrease code speed performance. Moreover, if you see _decompose_tensor_nan is just a generalization of decompose_tensor. I would also prefer not giving a default value for tensor_alternative - since this should be used for non-linear approaches the best tensor alternative is always the parameters initial estimate.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Nov 1, 2016

Minor corrections done! @arokem let me know about the other 2 points - I will resume work tomorrow.

@coveralls

This comment has been minimized.

coveralls commented Nov 1, 2016

Coverage Status

Coverage increased (+0.02%) to 87.167% when pulling 091c7cf on RafaelNH:fix_fwdti_eigenvalues into d4f45f2 on nipy:master.

@arokem

arokem approved these changes Nov 2, 2016

@arokem

This comment has been minimized.

Member

arokem commented Nov 2, 2016

👍 for the merge on my end. If someone else wants to take a look, I will give you a few days before going ahead with it.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Nov 14, 2016

Hi @arokem - should we merge this?

@arokem

This comment has been minimized.

Member

arokem commented Nov 14, 2016

Sorry for the delay. In it goes!

@arokem arokem merged commit 5c3812b into nipy:master Nov 14, 2016

4 checks passed

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