problem with Trace measurement from fiber tracts and Spherical Measure about slicerdmri HOT 10 CLOSED

Fan can you take an initial look at this on Monday and we can see if it's a quick fix?

from slicerdmri.

Also fyi I found this in the data Lily measured, so I am not sure what Slicer version she is using but I believe it is recent--you probably know.

from slicerdmri.

I found the issue, I think. When trace is computed from eigenvalues, for some reason NAN can be returned if any eigenvalue is below VTK eps (see below). However the eigenvalues are just returned directly. I see no good reason for this code to ever return NAN. I think the usage of VTK_EPS is an error because eigenvalues are expected to be very small numbers.

double vtkDiffusionTensorMathematics::Trace(double D[3][3])
{
return (D[0][0] + D[1][1] + D[2][2]);

}

double vtkDiffusionTensorMathematics::Trace(double w[3])
{
if ((w[0] <= VTK_EPS) || (w[1] <= VTK_EPS) || (w[2] <= VTK_EPS))
{
return DOUBLE_NAN;
}

return ( w[0] + w[1] + w[2] );
}

This also happens in spherical measure:
double vtkDiffusionTensorMathematics::SphericalMeasure(double w[3])
{
return OUT_OF_RANGE_TO_NAN((w[2]) / MAX(w[0], VTK_EPS), 0, 1);
}

And also linear and planar measure.

from slicerdmri.

There is some additional numerical issue happening. In the code, #define VTK_EPS 1e-16

but in my measurements, for example the eigenvalues are:
0.00078 0.000348 0.000348.
and the trace is reported as NAN.

None of these eigenvalues should be lower than the defined EPS. There may be something odd happening in the code when the model has the two smaller eigenvalues equal, or something...

We need this to work properly for studies happening now.

from slicerdmri.

Hi @ljod

There are NANs in the Autism data as well. I extracted Max, Min, Median, and Mean from each fiber bundle. For some bundles, while the first three are non-NAN values, the Mean has NAN. This means there are some points in these bundles are NAN so we get an overall NAN when averaging them.
(This function computes the average value of all points in a fiber bundle to get the output for a certain measurement: https://github.com/SlicerDMRI/SlicerDMRI/blob/master/Modules/CLI/FiberTractMeasurements/FiberTractMeasurements.cxx#L366)

In your last post, are those eigenvalues from the output of this module? If yes, they are the mean values of all points in the fiber bundle. There could be some points below VTK_EPS, since there is no VTK_EPS constraint for the eigenvalues (see below). But, when computing trace, the ones below VTK_EPS are set to NAN. Does this make sense?

double vtkDiffusionTensorMathematics::MaxEigenvalue(double w[3])
{
return w[0];
}
double vtkDiffusionTensorMathematics::MiddleEigenvalue(double w[3])
{
return w[1];
}
double vtkDiffusionTensorMathematics::MinEigenvalue(double w[3])
{
return w[2];
}

Regards,
Fan

from slicerdmri.

Good point that this is the average eigenvalue and average trace over the whole bundle. I had forgotten about that when I was looking at the code.

But still the trace should be the sum of the average eigenvalues. Trace should never equal NAN if the eigenvalues do not. Conceptually there is a problem here if these values do not correspond. If there are three average eigenvalues the trace should always be their sum. Otherwise there is a bug.

Having one or a few eigenvalues below VTK_EPS, should not break the measurement of the trace of the entire bundle. The eigenvalues are ok because they are not checked against VTK_EPS. The solution should be to get rid of the VTK_EPS in the computation of the trace.

from slicerdmri.

Yes, I agree with you!!

from slicerdmri.

:)
I wonder why the VTK_EPS was in the trace in the first place. I understand some parts of the code avoid crashes by checking eigenvalues do not equal zero or go negative, but the trace is safe to compute and having two ways to compute the trace that return two different values seems wrong to me.

I will see if Isaiah can submit a pull request soon so we can re-measure everything.

from slicerdmri.

For reference, the VTK_EPS check was added here to fix a bug (I haven't been able to locate original yet). I agree that it should be consistent between the two computations, and we should eliminate the repetition.

I stepped through in debugger at the point where the NaN is returned for a number of the clusters, and looked at the failing points. They are all due to negative eigenvalues. Also loaded in python, pulled out the corresponding tensors at those points, and calculated the eigenvals/vecs with Numpy. They are "close enough" (different at ~40th decimal place) so I think the calculations are fine.

For the purposes of deciding on excluding NaN from the current measurements, I checked the number of points w/ negative eigenvalues in the three largest clusters in the 01408_reg_reg_outlier_removed directory: only 125 (out of 484407 points). So based on that rate, it seems reasonable to just exclude the NaNs from the average calculation for now, as done here: 3357ec9

I am building a version of Slicer with this change on the cluster so that Lily can run it tomorrow. Before we make a more permanent change, I want to discuss again w/ @ljod and figure out if we should set a threshold for invalidating the results.

from slicerdmri.

Looks to me in the code like before, if negative eigenvalues were present, trace could go negative, and that somehow looked bad with the preset LUT for scalar range. Seems totally visualization motivated. Not designed for quantitative measures. Hope this helps.

On Mon, Feb 29, 2016 at 4:39 PM -0800, "Isaiah" <[email protected]mailto:[email protected]> wrote:

For reference, the VTK_EPS check was added herehttps://github.com/Slicer/Slicer/commit/70acc9ea8 to fix a bug (I haven't been able to locate original yet). I agree that it should be consistent between the two computations, and we should eliminate the repetition.

I stepped through in debugger at the point where the NaN is returned for a number of the clusters, and looked at the failing points. They are all due to negative eigenvalues. Also loaded in python, pulled out the corresponding tensors based, and calculated the eigenvals/vecs with Numpy. They are "close enough" (different at ~40th decimal place) so I think the calculations are fine.

For the purposes of deciding on excluding NaN from the current measurements, I checked the number of points w/ negative eigenvalues in the three largest clusters in the 01408_reg_reg_outlier_removed directory: only 125 (out of 484407 points). So based on that rate, it seems reasonable to just exclude the NaNs from the average calculation for now, as done here: 3357ec93357ec9

I am building a version of Slicer with this change on the cluster so that Lily can run it tomorrow. Before we make a more permanent change, I want to discuss again w/ @ljodhttps://github.com/ljod and figure out if we should set a threshold for invalidating the results.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/34#issuecomment-190470818.

The information in this e-mail is intended only for the person to whom it is
addressed. If you believe this e-mail was sent to you in error and the e-mail
contains patient information, please contact the Partners Compliance HelpLine at
http://www.partners.org/complianceline . If the e-mail was sent to you in error
but does not contain patient information, please contact the sender and properly
dispose of the e-mail.

from slicerdmri.