ENH: Add "itkPointSetTest.py" unittest, testing `SetPointsByCoordinates` #4860

N-Dekker · 2024-09-24T18:40:11Z

Specifically tested SetPointsByCoordinates with a NumPy array as input argument, as was suggested by @PranjalSahu.

Follow-up to pull request Add SetPointsByCoordinates to PointSet, add warning to SetPoints(PointsVectorContainer *) #4850 commit f43b1b8
ENH: Add PointSet::SetPointsByCoordinates(coordinates)

Specifically tested `SetPointsByCoordinates` with a NumPy array as input argument, as was suggested by Pranjal Sahu. - Follow-up to pull request InsightSoftwareConsortium#4850 commit f43b1b8 ENH: Add `PointSet::SetPointsByCoordinates(coordinates)`

dzenanz · 2024-09-24T21:17:27Z

/azp.run ITK.macOS.Arm64

dzenanz · 2024-09-24T21:17:59Z

/azp run ITK.macOS.Arm64

dzenanz · 2024-09-24T21:19:11Z

/azp run ITK.macOS.Python

dzenanz · 2024-09-24T21:19:53Z

/azp run ITK.macOS

N-Dekker · 2024-09-25T11:13:41Z

/azp run ITK.macOS.Arm64

@dzenanz FYI "ITK.macOS.Arm64"does not listen to /azp run, because it's not using Azure Pipelines!

I just pressed "Re-run jobs" for this GitHub Action, at https://github.com/InsightSoftwareConsortium/ITK/actions/runs/11019940074/job/30603577840?pr=4860

N-Dekker · 2024-09-25T12:32:32Z

Just checked, the test has actually run, and passed successfully: https://open.cdash.org/tests/1733705147

Test: itkPointSetTest (Passed)

Command Line:

/home/vsts/work/1/s-build/Wrapping/Generators/Python/itk/itkTestDriver "--add-before-env" "PYTHONPATH" "/home/vsts/work/1/s-build/Wrapping/Generators/Python" "--add-before-env" "PYTHONPATH" "/home/vsts/work/1/s-build/Wrapping/Generators/Python/itk" "--add-before-libpath" "/home/vsts/work/1/s-build/Wrapping/Generators/Python/itk" "/opt/hostedtoolcache/Python/3.9.20/x64/bin/python3" "/home/vsts/work/1/s/Modules/Core/Common/wrapping/test/itkPointSetTest.py"

👍

My very first ITK Python unittest 😇

PranjalSahu · 2024-09-25T20:53:21Z

Instead of Unsigned Char, can we instead use float ?
Because that will be the most used datatype.

PranjalSahu · 2024-09-25T21:00:14Z

We should also replace all the older tests using SetPoints with the new method to ensure everything else is working.

N-Dekker · 2024-09-25T21:44:50Z

Modules/Core/Common/wrapping/test/itkPointSetTest.py

+        # The pixel type is irrelevant for this test, just use unsigned char.
+        PixelType = itk.UC


Instead of Unsigned Char, can we instead use float ? Because that will be the most used datatype.

@PranjalSahu Thanks for the suggestion, but isn't UC a common pixel type as well? We could extend the test to try any possible pixel type, but it's irrelevant to the functionality of SetPointsByCoordinates anyway. SetPointsByCoordinates is just about the points of a PointSet. It's not about the pixels.

Note that this is not about the coordinate type. The coordinate type is defined by the "MeshTraits" of a PointSet type. (By default, it is 32-bit float, independent of the pixel type.)

For the record, by default, itk.PointSet supports double, float, short, unsigned char, unsigned int, itk::Array, itk::Matrix, itk::Point, and itk::Vector as pixel type, according to itk.PointSet.GetTypes():

>>> print(itk.PointSet.GetTypes()) <itkTemplate itk::PointSet> Options: [<itkCType double>, 2] [<itkCType double>, 3] [<itkCType float>, 2] [<itkCType float>, 3] [<itkCType signed short>, 2] [<itkCType signed short>, 3] [<itkCType unsigned char>, 2] [<itkCType unsigned char>, 3] [<itkCType unsigned int>, 2, <class 'itk.itkDefaultStaticMeshTraitsPython.itkDefaultStaticMeshTraitsUI22FFUI'>] [<itkCType unsigned int>, 3, <class 'itk.itkDefaultStaticMeshTraitsPython.itkDefaultStaticMeshTraitsUI33FFUI'>] [<class 'itk.itkArrayPython.itkArrayD'>, 2] [<class 'itk.itkArrayPython.itkArrayD'>, 3] [<class 'itk.itkMatrixPython.itkMatrixD22'>, 2] [<class 'itk.itkMatrixPython.itkMatrixD33'>, 3] [<class 'itk.itkPointPython.itkPointD2'>, 2, <class 'itk.itkDefaultStaticMeshTraitsPython.itkDefaultStaticMeshTraitsD22DDD'>] [<class 'itk.itkPointPython.itkPointD3'>, 3, <class 'itk.itkDefaultStaticMeshTraitsPython.itkDefaultStaticMeshTraitsD33DDD'>] [<class 'itk.itkVectorPython.itkVectorF3'>, 3] None

N-Dekker · 2024-09-25T21:48:40Z

We should also replace all the older tests using SetPoints with the new method to ensure everything else is working.

@PranjalSahu Please feel free to make another pull request to do so. (Otherwise I can also give it a try, once this pull request is merged.)

PranjalSahu

LGTM

thewtex

@N-Dekker thanks!

Request inline.

thewtex · 2024-09-26T16:19:23Z

Modules/Core/Common/wrapping/test/itkPointSetTest.py

+                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])


It is better to use numpy assert_allclose, which is must faster and has a shape check.

Thanks for the suggestion @thewtex Can you possibly type out exactly how you want it to be?

@thewtex Also, do you think the test should be faster than it is right now?

@thewtex Can you please show us exactly how you want to adjust this test, by using numpy assert_allclose? Otherwise can you please reconsider your change request? I would very much like this PR to be merged 😃

Maybe np.assert_allclose(points, np_array.T)? Does that compile and work?

Thanks @thewtex, but sorry, that does not work for me either. See below. Did you locally try this particular unit test with your suggested adjustment?

No

Would it be acceptable to you to just merge the pull request as it is right now, and then afterwards possibly make a new PR to further optimize the test, for example by using numpy.testing.assert_allclose?

No, this is an important performance and usage fix, and it is also important to verify that the proposed change is correct in terms of shape.

I will investigate locally.

it is also important to verify that the proposed change is correct in terms of shape.

@thewtex Regarding the shape, please note that it is important that the argument of SetPointsByCoordinates is a "flat" (1-D) array of coordinate values, when doing

point_set.SetPointsByCoordinates(np_array)

The SetPointsByCoordinates member function is meant to be an alternative to the existing unsafe SetPoints overload (issue #4848), which also expects a "flat" array of coordinate values as argument.

So the PointSet and the array of coordinates (np_array) have different shapes, but that is intentionally. So I'm sorry, I don't think numpy.testing.assert_allclose can be used for this unit test. 🤷

It might, with reshaping. Reshaping in numpy is easy.

I'm sorry, but this is just a single unit test. It is not meant to be a tutorial on how to reshape a numpy array.

The SetPointsByCoordinates member function is meant to be an alternative to the existing unsafe SetPoints overload (issue #4848), which also expects a "flat" array of coordinate values as argument.

So the PointSet and the array of coordinates (np_array) have different shapes, but that is intentionally.

The intension and/or the related support functions may need improvements :-). NumPy arrays representing points should not be flat -- they should have a dimension that corresponds to the spatial dimension.

N-Dekker · 2024-11-05T12:48:54Z

While the merged pull request ENH: Test Python wrapping with both SetPoints and SetPointsByCoordinates #4896 already extended the existing serialization tests in Python to use SetPointsByCoordinates, I still think it would be useful to specifically test SetPointsByCoordinates in isolation, as I'm proposing with this unit test.

dzenanz approved these changes Sep 24, 2024

View reviewed changes

N-Dekker marked this pull request as ready for review September 25, 2024 12:15

N-Dekker requested a review from PranjalSahu September 25, 2024 12:15

N-Dekker commented Sep 25, 2024

View reviewed changes

PranjalSahu approved these changes Sep 26, 2024

View reviewed changes

thewtex requested changes Sep 26, 2024

View reviewed changes

N-Dekker requested a review from thewtex October 7, 2024 14:49

N-Dekker mentioned this pull request Oct 21, 2024

BUG: Replace SetPoints with SetPointsByCoordinates in wrapping tests #4889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add "itkPointSetTest.py" unittest, testing `SetPointsByCoordinates` #4860

ENH: Add "itkPointSetTest.py" unittest, testing `SetPointsByCoordinates` #4860

N-Dekker commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

N-Dekker commented Sep 25, 2024 •

edited

Loading

N-Dekker commented Sep 25, 2024 •

edited

Loading

PranjalSahu commented Sep 25, 2024 •

edited

Loading

PranjalSahu commented Sep 25, 2024

N-Dekker Sep 25, 2024

N-Dekker Sep 26, 2024

N-Dekker commented Sep 25, 2024

PranjalSahu left a comment

thewtex left a comment

thewtex Sep 26, 2024

N-Dekker Sep 26, 2024

N-Dekker Sep 27, 2024

N-Dekker Oct 1, 2024

dzenanz Oct 1, 2024

thewtex Oct 7, 2024

N-Dekker Oct 8, 2024

dzenanz Oct 8, 2024

N-Dekker Oct 8, 2024

thewtex Oct 8, 2024

N-Dekker commented Nov 5, 2024

		# The pixel type is irrelevant for this test, just use unsigned char.
		PixelType = itk.UC

ENH: Add "itkPointSetTest.py" unittest, testing SetPointsByCoordinates #4860

Are you sure you want to change the base?

ENH: Add "itkPointSetTest.py" unittest, testing SetPointsByCoordinates #4860

Conversation

N-Dekker commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

dzenanz commented Sep 24, 2024

N-Dekker commented Sep 25, 2024 • edited Loading

N-Dekker commented Sep 25, 2024 • edited Loading

PranjalSahu commented Sep 25, 2024 • edited Loading

PranjalSahu commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker commented Sep 25, 2024

PranjalSahu left a comment

Choose a reason for hiding this comment

thewtex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker commented Nov 5, 2024

ENH: Add "itkPointSetTest.py" unittest, testing `SetPointsByCoordinates` #4860

ENH: Add "itkPointSetTest.py" unittest, testing `SetPointsByCoordinates` #4860

N-Dekker commented Sep 25, 2024 •

edited

Loading

N-Dekker commented Sep 25, 2024 •

edited

Loading

PranjalSahu commented Sep 25, 2024 •

edited

Loading