Fitted coefficient not returned if meanSquareError is null for ApprParabola2

[JulienMaille]

Hello Dave, I may have found an issue in the code below:
Solved coefficient are in vecU and they are not copied into u

GeometricTools/GTE/Mathematics/ApprParabola2.h

Lines 89 to 92 in 768c731

	bool success = LinearSystem<T>().Solve(A, B, vecU);
	if (success && meanSquareError != nullptr)
	{
	u = { vecU[0], vecU[1], vecU[2] };

Oh and I suppose there is a typo here on the second u0

GeometricTools/GTE/Mathematics/ApprParabola2.h

Line 34 in 768c731

// Fit with y = u0*x^2 + u1*x + u0

[davideberly]

I am out of the office for a couple of days, so no access to my development environment. My fix is to delete the test of meanSquareError so that you get into the block to assign vecU to u. The test for meanSquareError should contain the block where I compute totalError and then assign it to *meanSquareError. The idea was to allow you to use the fitting code without cost of computing the error (unless you want to know the error). I looked at my unit tests and saw that I apparently was only concerned about quality of fit; all the tests have a nonnull meanSquareError. I will modify the unit tests to include a test for when the pointer is null. I will also fix the typographical error in the comment.

Thank you for taking the time to report this

[JulienMaille]

Hello, you can look at my pull request for a solution that mimics what you have done for other fitting routines

[davideberly]

Sorry, the GitHub interface in my browser would not give me the ability to look at a diff. Also, the check-in number next to your change about ApprParabola2.h should be a link to the changes you made. Instead, I was taken to the file I modified last. After digging around the interface, I finally got a split diff. What you have is fine with me. Before I approve and merge, would you mind modifying the file version in the header at the top of the file? Just choose whatever date you make the change. I use the file version in case someone reports a problem that was fixed in a file with an earlier version. Thank you.

[JulienMaille]

Done

[davideberly]

Thanks. I have posted the fix to Fit(...). The same modification needed to be made to FitRobust(...). The check-in comments:

If meanSquareError is null in the call to Fit, the local vecU is not copied to the output u. Thanks to Julien Maille for reporting and fixing the bug. The same bug occurs in FitRobust and needed to be fixed. The latest code has both fixes. The previous code uses a linear system solver specialized for 3x3 matrix systems. The fixes use a linear system solver with general purpose Gaussian elimination. This will result in lightly slower performance, but could be costly if you call the fitter a large number of times. The ApprParaboloid3 class has similar logic and uses the general purpose Gaussian elimination solely because GTE does not contain a specialization for 6x6 matrix systems. (I have added to my TODO list to use only specialized matrix system solvers for the
fitting.)

[JulienMaille]

Oh I had not understood that the change would slow down the fit, that defeats the whole purpose of skipping. Should I start another PR and provide a much simpler fix?
from

bool success = LinearSystem<T>().Solve(A, B, vecU);
if (success && meanSquareError != nullptr)
{
   u = { vecU[0], vecU[1], vecU[2] };
   [...]

to

bool success = LinearSystem<T>().Solve(A, B, vecU);
u = { vecU[0], vecU[1], vecU[2] };
if (success && meanSquareError != nullptr)
{
   [...]

[davideberly]

I will post the change I had intended to fix the bug and retain good performance. I will also look at an alternative for the ApprParaboloid3 class that you have used before. The 6x6 matrix is symmetric, so the LDL^T decomposition might perform better than the general-purpose Gaussian elimination. The ultimate test for both is to run a profiler (I use Intel V-Tune). This work will not take that long.