-
-
Notifications
You must be signed in to change notification settings - Fork 663
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
BUG: Replace SetPoints
with SetPointsByCoordinates
in wrapping tests
#4889
Conversation
When the points are specified by a flat VectorContainer of coordinates, passing those points to `SetPoints` may lead to undefined behavior. `SetPointsByCoordinates` is in that case preferred. - Partially addresses issue InsightSoftwareConsortium#4848 "`PointSet::SetPoints(PointsVectorContainer *)` overload leads to undefined behavior"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The OAI project needs to be updated for newer version of ITK.
Here, itk_mesh.SetPoints(itk.vector_container_from_array(points_numpy))
@@ -52,7 +52,7 @@ | |||
arr = itk.array_view_from_vector_container(v_point) | |||
points_vc = itk.vector_container_from_array(arr.flatten()) | |||
|
|||
point_set.SetPoints(points_vc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this "in addition" instead of "instead of" to improve coverage, i.e.,
point_set.SetPoints(points_vc)
point_set.SetPointsByCoordinates(points_vc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thewtex I see your point; this pull request would take away code coverage from PointSet::SetPoints(PointsVectorContainer *)
. But then again, I think we should eventually deprecate this specific SetPoints
overload anyway, to avoid undefined behavior (issue #4848).
If we still want to have this specific SetPoints
overload around for a while, I would prefer a separate isolated test for this specific overload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also do:
if test_unsafe_SetPoints_overload:
point_set.SetPoints(points_vc)
else:
point_set.SetPointsByCoordinates(points_vc)
And then run the test for both test_unsafe_SetPoints_overload = True
and test_unsafe_SetPoints_overload = False
. Would you like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@N-Dekker yes, that is a good approach to both indicate that the code is unsafe and keep the coverage.
|
|
When the points are specified by a flat VectorContainer of coordinates, passing those points to
SetPoints
may lead to undefined behavior.SetPointsByCoordinates
is in that case preferred.PointSet::SetPoints(PointsVectorContainer *)
overload leads to undefined behavior #4848SetPointsByCoordinates
#4860Aims to address a comment by @PranjalSahu at #4860 (comment) saying: