ContourSpatialObject<TDimension>::Update() LINEAR_INTERPOLATION case may need some adjustment

[N-Dekker]

There is a line of code in ContourSpatialObject<TDimension>::Update() that looks suspicious to me:

newPoint = pnt[d] + i * step[d]; 

ITK/Modules/Core/SpatialObjects/include/itkContourSpatialObject.hxx

Lines 217 to 225 in a2b3915

	PointType newPoint;
	newPoint.Fill(NumericTraits<double>::max());
	for (unsigned int i = 0; i < m_InterpolationFactor; ++i)
	{
	for (unsigned int d = 0; d < TDimension; ++d)
	{
	newPoint = pnt[d] + i * step[d];
	}
	}

You see, newPoint is a PointType, and it gets filled by a scalar value, pnt[d] + i * step[d], over and over again, inside the inner for loop.

I think the intention was to do:

newPoint[d] = pnt[d] + i * step[d]; 

But I'm not sure, because I did not study the code well enough yet! Any feedback or possible explanation is welcome!

Note: This bug report is still in progress! More details soon, hopefully!

[dzenanz]

@aylward might have some comment.

[aylward]

You're right! Great catch! Want to submit a pull request to fix it?

[N-Dekker]

Great catch! Want to submit a pull request to fix it?

Thanks @aylward but I don't know how to fix it properly. Is it really a serious bug? Or is this part of the code not so important anyway?

If it is a serious bug, can you please tell me the intended behavior?

[N-Dekker]

The apparent bug is part of the case InterpolationMethodEnum::LINEAR_INTERPOLATION section of a switch (m_InterpolationMethod). It appears that this specific "case" is not tested, at least not by itkContourSpatialObjectTest.cxx. If it is indeed untested, and if it is also unused, I would prefer to just put some itkExceptionMacro(<< "not yet defined") call there, just like in case InterpolationMethodEnum::BEZIER_INTERPOLATION. Then postpone actual implementation until it is really needed. Would that be OK to you?

ITK/Modules/Core/SpatialObjects/include/itkContourSpatialObject.hxx

Lines 183 to 193 in a2b3915

	this->SetPoints(m_ControlPoints);
	break;
	case InterpolationMethodEnum::EXPLICIT_INTERPOLATION:
	break;
	case InterpolationMethodEnum::BEZIER_INTERPOLATION:
	// TODO: Implement bezier interpolation
	{
	itkExceptionMacro(<< "Bezier interpolation type not yet defined.");
	}
	break;
	case InterpolationMethodEnum::LINEAR_INTERPOLATION:

[aylward]

No. Some kind of interpolation is needed to create a contour from control points. I'd rather add a test and fix it. It is a one line change to fix, exactly as you noted. I will get to it after March 8th. I have another project due that day.

…

On Wed, Feb 23, 2022, 5:03 PM Niels Dekker ***@***.***> wrote: The apparent bug is part of the case InterpolationMethodEnum::LINEAR_INTERPOLATION section of a switch (m_InterpolationMethod). It appears that this specific "case" is not tested, at least not by itkContourSpatialObjectTest.cxx <https://github.com/InsightSoftwareConsortium/ITK/blob/a2b39155dae42fb88d52c0d7d7d7e1f1a8c3e954/Modules/Core/SpatialObjects/test/itkContourSpatialObjectTest.cxx>. If it is indeed untested, and if it is also unused, I would prefer to just put some itkExceptionMacro(<< "not yet defined") call there, just like in case InterpolationMethodEnum::BEZIER_INTERPOLATION. Then postpone actual implementation until it is really needed. Would that be OK to you? https://github.com/InsightSoftwareConsortium/ITK/blob/a2b39155dae42fb88d52c0d7d7d7e1f1a8c3e954/Modules/Core/SpatialObjects/include/itkContourSpatialObject.hxx#L183-L193 — Reply to this email directly, view it on GitHub <#3222 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEJL6WIPJEREAM73E42X3U4VKRJANCNFSM5PFGPPPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[N-Dekker]

@aylward Thanks for your reply, Stephen. Please check pull request #3225 "BUG: Quick-fix ContourSpatialObject::Update(), LINEAR_INTERPOLATION case", and if possible, please approve the pull request 😃

I think it would still be useful if you could get back to it after March 8th, because I consider my pull request #3225 just a quick-fix. I guess you may be able to fix it more thoroughly 😃

[N-Dekker]

I'm glad to see the quick-fix for this issue is merged quickly, thanks @hjmjohnson! So now the code says:

 PointType newPoint; 
 newPoint.Fill(NumericTraits<double>::max()); 
 for (unsigned int i = 0; i < m_InterpolationFactor; ++i) 
 { 
   for (unsigned int d = 0; d < TDimension; ++d) 
   { 
     newPoint[d] = pnt[d] + i * step[d]; 
   } 
 } 

ITK/Modules/Core/SpatialObjects/include/itkContourSpatialObject.hxx

Lines 222 to 230 in 5b71d63

	PointType newPoint;
	newPoint.Fill(NumericTraits<double>::max());
	for (unsigned int i = 0; i < m_InterpolationFactor; ++i)
	{
	for (unsigned int d = 0; d < TDimension; ++d)
	{
	newPoint[d] = pnt[d] + i * step[d];
	}
	}

My remaining questions are:

• Why does it do a newPoint.Fill(...), when it appears that each coordinate of newPoint will get a new value in the following lines of code. Is that means to support m_InterpolationFactor == 0?
• Why would the newPoint be modified multiple times, inside the for (unsigned int i = 0; i < m_InterpolationFactor; ++i) loop? It seems to me that only the result of the last iteration (i == m_InterpolationFactor - 1) is relevant, right?

Anyway, no hurry, I can wait until after after March 8th 😃