ITK icon indicating copy to clipboard operation
ITK copied to clipboard

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

Open N-Dekker opened this issue 1 year ago • 9 comments

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

  • Follow-up to pull request https://github.com/InsightSoftwareConsortium/ITK/pull/4850 commit f43b1b8977fa05d169c14b4c6f44ce35992a3959 ENH: Add PointSet::SetPointsByCoordinates(coordinates)

N-Dekker avatar Sep 24 '24 18:09 N-Dekker

/azp.run ITK.macOS.Arm64

dzenanz avatar Sep 24 '24 21:09 dzenanz

/azp run ITK.macOS.Arm64

dzenanz avatar Sep 24 '24 21:09 dzenanz

/azp run ITK.macOS.Python

dzenanz avatar Sep 24 '24 21:09 dzenanz

/azp run ITK.macOS

dzenanz avatar Sep 24 '24 21:09 dzenanz

/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 avatar Sep 25 '24 11:09 N-Dekker

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 😇

N-Dekker avatar Sep 25 '24 12:09 N-Dekker

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

PranjalSahu avatar Sep 25 '24 20:09 PranjalSahu

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

PranjalSahu avatar Sep 25 '24 21:09 PranjalSahu

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.)

N-Dekker avatar Sep 25 '24 21:09 N-Dekker

  • While the merged pull request #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.

N-Dekker avatar Nov 05 '24 12:11 N-Dekker

@N-Dekker Pull requests do not seem to be available for your branch, but a few improvements are added to your branch here:

https://github.com/thewtex/ITK/tree/itkPointSetTest-Python-unittest

which adds support for passing a n_points x dim to SetPointsByCoordinates (which is what I think people expect from a Python interface). It also adds a GetPointsByCoordinates that returns the same. And verifies with numpy.testing.assert_allclose.

thewtex avatar Dec 10 '24 23:12 thewtex

https://github.com/thewtex/ITK/tree/itkPointSetTest-Python-unittest

which adds support for passing a n_points x dim to SetPointsByCoordinates (which is what I think people expect from a Python interface). It also adds a GetPointsByCoordinates that returns the same. And verifies with numpy.testing.assert_allclose.

Thanks Matt! I think it's very nice to add support for 2D (number of points x dimension) numpy arrays. The approach of using %rename and %extend swig_name is new to me, cool!

I'm glad to see your proposal still preserves the original support of flat arrays of coordinates as was added by PR #4850 and included with ITK v6.0a01. (I think that was an essential feature, in order to provide an alternative to the problematical SetPoints overload.) So your proposal looks like an enhancement on top of PR #4850 and #4860 (this one), right? As such, I would rather see this one (PR #4860) being merged, so that we can then review your proposal as a follow-up PR. Would that be OK to you?

N-Dekker avatar Dec 11 '24 16:12 N-Dekker

:+1: yes, @N-Dekker sounds like a good plan.

thewtex avatar Dec 11 '24 19:12 thewtex