pointsetbase set get numpy arrays #5045
Conversation
thewtex
commented
Dec 11, 2024
thewtex
commented
- STYLE: Use Python in Common python test names
- ENH: Shaped NumPy array input to SetPointsByCoordinates
Follow the convention to end in "PythonTest" for python tests to help identifying the tests. We had, for example, two tests nameed itkPointSetTest (one C++ and one Python).
Accept and n_points by dimension shaped NumPy array input to GetPointsByCoordinates, which is the expected shape for point coordinates. Continue to also support flat array inputs. Also add a corresponding shaped NumPy output in a GetPointsByCoordinates method. Add GetPointDimension() to PointSetBase. This provides access to the point dimension at runtime for GetPointsByCoordinates and is similar to GetImageDimension() on ImageBase.
github-actions
bot
added
type:Infrastructure
hjmjohnson
merged commit 1790947
into
InsightSoftwareConsortium:master
|
@hjmjohnson Why is this PR merged so quickly, while my PR #4860 literally took months to get merged? I would have liked to get an opportunity to review this PR, especially because it adjusts the code that I originally contributed. Also, Matt assigned me as one of the reviewers. |
| /** Point dimension. The dimension of a point is fixed at compile-time. */ | ||
| static constexpr unsigned int | ||
| GetPointDimension() | ||
| { | ||
| return PointDimension; | ||
| } | ||
|
|
There was a problem hiding this comment.
This member function seems redundant, as PointDimension is already a public member. Are static constexpr public data members (like PointDimension) not made available already, by SWIG?
There was a problem hiding this comment.
@N-Dekker correct -- Python only has access to the run-time methods.
There was a problem hiding this comment.
Thanks Matt. It's a pity though, if SWIG cannot wrap them automatically. So just to be sure, I asked the question at swig/discussions as well:
There was a problem hiding this comment.
On a high level, as a runtime language, Python has access to runtime information.
There was a problem hiding this comment.
The reply from William S Fulton (@wsfulton) at swig/swig#3088 (comment) suggests that it is possible to directly get the value of a static constexpr data member of a template class, in Python. (So then the GetPointDimension() member function that you added should not be necessary.) However, I haven't yet been able to figure out how to make it work. 🤷 To be continued, hopefully...
| for i in range(number_of_points): | ||
| point = points.GetElement(i) | ||
| for j in range(dimension): | ||
| self.assertAlmostEqual(point[j], np_array[i * dimension + j]) |
There was a problem hiding this comment.
I think this is a good test for the flat array case. I think it should not have been removed.
There was a problem hiding this comment.
We do retain testing for the flat array input. IT has the same output as the shaped array, and it is tested with the more detailed and efficient method.
There was a problem hiding this comment.
Thanks Matt, but in order to properly test for the flat array input, the asserts should be placed directly after the SetPointsByCoordinates(<flat array>) call. Not just after the SetPointsByCoordinates(<shaped array>) call.
There was a problem hiding this comment.
We can also check the return value after passing the flat array: #5056
| def GetPointsByCoordinates(self): | ||
| """Get the points of the pointset by providing their coordinates | ||
| as a NumPy array. The array will have a shape of (n_points, dimension).""" |
There was a problem hiding this comment.
I'm not sure about the name "GetPointsByCoordinates". Shouldn't it be something like "GetPointsAsNumpyArray"? Or "converts_points_to_np_array"?
There was a problem hiding this comment.
I think it makes most sense to retain parity of SetPointsByCoordinates with GetPointsByCoordinates.
There was a problem hiding this comment.
OK, but we could then also rename the new SetPointsByCoordinates. My original intention was to express that SetPointsByCoordinates sets the points by specifying a simple sequence of coordinates. For the newly added functionality, SetPointsByNumpyArray (or set_points_by_np_array) might be a more appropriate name, right?
There was a problem hiding this comment.
Isn't the API getting unnecessarily complicated?
I thought that you discouraged the use of SetPoints?
There was a problem hiding this comment.
Isn't the API getting unnecessarily complicated?
When two functions do different things, I think it is clearer to also use different names for those functions. The original SetPoints(PointsContainer *) (which has been there for many, many years already, including ITK 1, 2, 3, 4, 5, and 6 alpha 1) shares the container between the caller and the PointSet. The new SetPointsByCoordinates(const std::vector<CoordinateType> &) copies the coordinates from the caller into the container of the PointSet.
I thought that you discouraged the use of
SetPoints?
The original SetPoints(PointsContainer *) is OK to me. It's OK to "share" your container with a point set, if that's what you want. However, the extra SetPoints(PointsVectorContainer *) overload that was added with ITK v5.3.0 appears problematic, as I explained at issue #4848 If I understand correctly, the extra SetPoints(PointsVectorContainer *) overload was specifically introduced so support flat coordinate arrays. But unfortunately it may lead to undefined behavior. The new SetPointsByCoordinates(const std::vector<CoordinateType> &) was just meant to provide a safe alternative to the problematic SetPoints(PointsVectorContainer *) overload of ITK >= v5.3.0.
Is that a helpful explanation to you?
There was a problem hiding this comment.
@N-Dekker I understand what you mean. However, I do not see the need for another method, which over-complicates the API. It is standard practice in object-oriented APIs to have the same method accept two different types.
