Add SetPointsByCoordinates to PointSet, add warning to SetPoints(PointsVectorContainer *)

[N-Dekker]

• Added a warning to PointSet::SetPoints(PointsVectorContainer *), mitigating issue PointSet::SetPoints(PointsVectorContainer *) overload leads to undefined behavior #4848
• Added PointSet::SetPointsByCoordinates as a safe alternative.

[dzenanz]

If I remember correctly, the main point of the old method was convenient usage from Python. @PranjalSahu is that correct?

@N-Dekker how would use of the proposed method look like from Python? Perhaps write a Python test to demonstrate that?

[N-Dekker]

Perhaps write a Python test to demonstrate that?

I'm sorry I'm not very good in Python tests. 🤷 But it looks like SWIG supports passing a Python tuple or list as argument, when the parameter type is std::vector.

[PranjalSahu]

If I remember correctly, the main point of the old method was convenient usage from Python. @PranjalSahu is that correct?

@N-Dekker how would use of the proposed method look like from Python? Perhaps write a Python test to demonstrate that?

Yes, another library supported such functionality of directly passing a 1-D vector of points in any dimension, so we wanted to have a similar feature here. It would be better to find an alternative to reinterpret_cast instead of adding a new method.
Another reason is to support Numpy vector for setting the points.

If there are no alternatives then yes we can add a new method.

Also did @N-Dekker did you come across any platform where this method is breaking?

[N-Dekker]

did you come across any platform where this method is breaking?

Honestly, I did not. But it's really quite a coincidence that the reinterpret_cast<PointsContainer *>(points) just seems to work so far. As you know, it internally "reinterprets" the bits of an std::vector<CoordRepType> as an std::vector<PointType>. Suppose the std::vector would internally have a m_NumberOfElements member variable. This member would then have the same value after the "reinterpretation", as before. But that would be wrong, of course. The number of elements of std::vector<PointType> is less than the number of elements of std::vector<CoordRepType>.

In general, undefined behavior should be avoided because the compiler (especially the optimizer) may mess up things as it is allowed to assume that there is no undefined behavior.

[N-Dekker]

It would be better to find an alternative to reinterpret_cast instead of adding a new method.

My idea would be to deprecate SetPoints(PointsVectorContainer *) and suggest users to use SetPointsByCoordinates instead.

Why would you not want to have SetPointsByCoordinates(const std::vector<CoordRepType> &) ?

[PranjalSahu]

ok it will be great if we could create a scenario where it fails!

My understanding is that it is converting the pointer type of the continuous memory block where the 1D vector of 1D points of length 3N is stored, to a vector of length N of 3D Points. Essentially the same as Numpy reshape operator (1 x 3N -> 3xN).

This is needed because only 1D vectors are supported while converting the Numpy vector to VectorContainer and vice-versa.

[N-Dekker]

Interesting errors at https://open.cdash.org/viewBuildError.php?buildid=9909255, please have a look:

/.../s/Modules/Core/Common/include/itkPointSet.hxx: In instantiation of 'void itk::PointSet<TPixelType, VDimension, TMeshTraits>::SetPointsByCoordinates(const std::vector<typename TMeshTraits::CoordRepType>&) [with TPixelType = double; unsigned int VDimension = 2; TMeshTraits = itk::QuadEdgeMeshTraits<double, 2, bool, bool, float, float>; typename TMeshTraits::CoordRepType = float]':
Wrapping/Modules/ITKQuadEdgeMesh/itkQuadEdgeMeshBasePython.cpp:11546:97:   required from here

Modules/Core/Common/include/itkPointSet.hxx:109:31: error: 'const struct std::pair<const long unsigned int, itk::QuadEdgeMeshPoint<float, 2, itk::GeometricalQuadEdge<long unsigned int, long unsigned int, bool, bool, true> > >' has no member named 'cbegin'

It appears that PointsContainer may be an itk::MapContainer, rather than just an itk::VectorContainer!

ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshTraits.h

Line 87 in 9a5dade

using PointsContainer = MapContainer<PointIdentifier, PointType>;

Obviously reinterpret_cast<PointsContainer *>(points) won't work when PointsContainer is an instance of itk::MapContainer. But my pull request should also still be adjusted to support itk::MapContainer, apparently 🤷

[N-Dekker]

@PranjalSahu Please try the following:

#include "itkPointSet.h"
#include "../../QuadEdgeMesh/include/itkQuadEdgeMeshTraits.h"
#include <gtest/gtest.h>

TEST(PointSet, SetPoints)
{
  static constexpr auto PointDimension = 2;

  using MeshTraits = itk::QuadEdgeMeshTraits<double, PointDimension, bool, bool>;
  using PointSetType = itk::PointSet<double, PointDimension, MeshTraits>;
  using CoordRepType = PointSetType::CoordRepType;

  auto pointSet = PointSetType::New();
  auto pointsVectorContainer = PointSetType::PointsVectorContainer::New();
  auto numberOfPoints = 42;
  pointsVectorContainer->CastToSTLContainer() = std::vector<CoordRepType>(numberOfPoints * PointDimension);
  pointSet->SetPoints(pointsVectorContainer);
  auto points = pointSet->GetPoints();

  EXPECT_EQ(points->size(), numberOfPoints);
}

It appears that after calling your SetPoints(PointsVectorContainer *) overload, the point-set has got an arbitrary number of points. While it should have been 42! 🤷 (Don't blame me for the MeshTraits 😇 )

[PranjalSahu]

Ok yes, it was not designed to be used with such complex point types.
So yes, either we can create a new method as you suggest or add a comment to not use it with complex Point types.

[N-Dekker]

/azp run ITK.Linux.Python

[N-Dekker]

By the way, I proposed a different name for the added member function, "SetPointsByCoordinates", because it is fundamentally different from the two existing SetPoints overloads. Those overloads both "share" the ownership of the object passed as argument (using ITK's reference counting mechanism). Whereas "SetPointsByCoordinates" copies the coordinates from the specified argument.

Personally I think the name "SetPointsByCoordinates" is fine, but it could also be named, for example, "SetPointsFromCoordinates" or "CopyCoordinatesToPoints". A matter of taste, I guess 🤷

[PranjalSahu]

Yes, that was also one of the requirements. Because the number of points could be large, we wanted to avoid copying the data.

[N-Dekker]

Yes, that was also one of the requirements. Because the number of points could be large, we wanted to avoid copying the data.

How large could the number of points possibly be, in your application? Can you please give the SetPointsByCoordinates member function that I'm proposing a chance?

Undefined behavior is just not an option, for any professional application or any serious open source project.

[PranjalSahu]

We will have to add tests for Python wrapping also.

[N-Dekker]

We will have to add tests for Python wrapping also.

OK, thanks for reminding me, @PranjalSahu, but Python tests can also be added in a follow-up pull request. The given PR does not break any existing use case. While it does add a valid C++ use case, already.

(I like to keep pull requests small, so that they can be reviewed easily, and merged smoothly.)

---

PS My first impression is that passing an np.array([1.0, 2.0, 3.0]) argument to a Python wrapped C++ function whose parameter type is const std::vector<double>& just works well. 👍 (I just tried it for ITKElastix.)

[blowekamp]

This looks like a good and clean addition.

It only adds comments about an unsafe methods. These additional request for testing and compatibility ( python api?) could be done if the unsafe methods is marked for deprecation or removed in future work.

[PranjalSahu]

We will have to add tests for Python wrapping also.

OK, thanks for reminding me, @PranjalSahu, but Python tests can also be added in a follow-up pull request. The given PR does not break any existing use case. While it does add a valid C++ use case, already.

(I like to keep pull requests small, so that they can be reviewed easily, and merged smoothly.)

PS My first impression is that passing an np.array([1.0, 2.0, 3.0]) argument to a Python wrapped C++ function whose parameter type is const std::vector<double>& just works well. 👍 (I just tried it for ITKElastix.)

Before removing this it would be great if the Python wrapping could be tested and merged.
Because the only purpose to add the vectorcontainer support was for Python wrapping.

[N-Dekker]

Is there a Python wheel for Windows available, based on the latest commit, including this pull request? I would like to try out the new SetPointsByCoordinates member function in Python.

[dzenanz]

A Python wheel is not built automatically. You need a local build with wrapping enabled. Then, copy C:\Dev\ITK-build\Wrapping\Generators\Python\WrapITK.pth to your_venv/Lib/site-packages. After restarting Python interpreter ITK should be available.