Skip to content

ENH: Add itk::PointSetBase as abstract base class of itk::PointSet#4867

Merged
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:PointSetBase
Oct 7, 2024
Merged

ENH: Add itk::PointSetBase as abstract base class of itk::PointSet#4867
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:PointSetBase

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Oct 2, 2024

Allows an algorithm to process the points of a PointSet object, without having to know its PixelType at compile-time. The algorithm may then just use a pointer or reference to its abstract base class, PointSetBase. Doing so may reduce the "code bloat" caused by template instantiations, and ease wrapping the algorithm for Python.

With this commit, its PixelType and its PointData container are still defined inside itk::PointSet, while the other properties (its points and its region information) are moved to its base class, PointSetBase.

@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module labels Oct 2, 2024
@github-actions github-actions bot added the area:Python wrapping Python bindings for a class label Oct 2, 2024
@github-actions github-actions bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Oct 3, 2024
Allows an algorithm to process the points of a `PointSet` object, without
having to know its `PixelType` at compile-time. The algorithm may then just use
a pointer or reference to its abstract base class, `PointSetBase`. Doing so may
reduce the "code bloat" caused by template instantiations, and ease wrapping the
algorithm for Python.

With this commit, its `PixelType` and its `PointData` container are still
defined inside `itk::PointSet`, while the other properties (its points and
its region information) are moved to its base class, `PointSetBase`.
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 4, 2024

/azp run ITK.macOS.Python

@N-Dekker N-Dekker marked this pull request as ready for review October 4, 2024 09:29
@dzenanz dzenanz requested a review from blowekamp October 4, 2024 12:17
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 4, 2024

still preferable to wrap the base class before the derived class

Yes.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 4, 2024

FYI, my intention is to pave the way for adding functions like SetFixedPointSet(const PointSetBaseType * pointSet) and SetMovingPointSet(const PointSetBaseType * pointSet) to the itk::ElastixRegistrationMethod of elastix. These functions might then support PointSet objects of any PixelType or PointData.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Oct 4, 2024

I was wondering what was the main purpose of this refactoring. 😄

Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 🚀

@thewtex thewtex merged commit bf0ed18 into InsightSoftwareConsortium:master Oct 7, 2024
Comment on lines +22 to +27
itk_wrap_class("itk::PointSetBase" POINTER)
foreach(d ${ITK_WRAP_IMAGE_DIMS})
itk_wrap_template("MC${ITKM_IT}QEMPF${d}" "itk::MapContainer< ${ITKT_IT}, itk::QuadEdgeMeshPoint< float, ${d} > >")
endforeach()
itk_end_wrap_class()

Copy link
Copy Markdown
Contributor Author

@N-Dekker N-Dekker Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this PR successfully passed the CI, it now looks like it made ITK.Windows.Python unhappy! It says at https://open.cdash.org/viewBuildError.php?type=1&buildid=9948358

itkQuadEdgeMeshBase: warning(4): ITK type not wrapped, or currently not known: itk::PointSetBase< itk::MapContainer< unsigned long long, itk::QuadEdgeMeshPoint< float, 2, itk::GeometricalQuadEdge< unsigned long long, unsigned long long, bool, bool > > > >
itkQuadEdgeMeshBase: warning(4): ITK type not wrapped, or currently not known: itk::MapContainer< unsigned long long, itk::QuadEdgeMeshPoint< float, 2 > >
itkQuadEdgeMeshBase: warning(4): ITK type not wrapped, or currently not known: itk::QuadEdgeMeshPoint< float, 2 >

I tried to locally build ITK + Python wrapping, using VS2022, then I got:

------ Build started: Project: ITKQuadEdgeMeshPython, Configuration: Release x64 ------
Generating itkQuadEdgeMeshBasePython.cpp, ../../Generators/Python/itk/itkQuadEdgeMeshBasePython.py
Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Nothing known about base class 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >'. Ignored.
Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Maybe you forgot to instantiate 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >' using %template.
Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1304): warning 401: Nothing known about base class 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,3,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >'. Ignored.
Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1304): warning 401: Maybe you forgot to instantiate 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,3,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >' using %template.
C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): error MSB8066: Custom build for 'Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgeMeshBasePython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgeMeshLineCellPython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgeMeshPointPython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgeMeshToQuadEdgeMeshFilterPython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgeMeshTraitsPython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\itkQuadEdgePython.cpp.rule;Bin\ITK\CMakeFiles\e1de2d8bd1732bb23f7a07883dd8477b\ITKQuadEdgeMeshPython.cpp.rule' exited with code 1.

Any idea?

Update: I think the itk::MapContainer< ${ITKT_IT}... should have been a itk::MapContainer< unsigned long long..., it isn't clear to me what ${ITKT_IT} evaluates to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't ITKT_IT specifier for IdentifierType, which should in turn be unsigned long long on MSVC?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a quick look, nothing jumps out as wrong. Best way to debug this is to set up a local Python build, and then iteratively tweak the wrap file and build.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't ITKT_IT specifier for IdentifierType, which should in turn be unsigned long long on MSVC?

Thanks @dzenanz, looks like you're right 👍 I still wonder why itk::IdentifierType isn't just spelled out in the wrap files. "${ITKT_IT}" isn't very readable, is it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of those ITKT_IT-style specifiers are abbreviated. The list is here: https://itk.org/ITKSoftwareGuide/html/Book1/ITKSoftwareGuide-Book1ch9.html#x55-1520009.5

See also a PR which fixes ITKT_IT: InsightSoftwareConsortium/ITKSoftwareGuide#215

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dzenanz I still find it hard to grasp all those ${ITKA_B}...${ITKY_Z} abbreviations, but maybe it's just me 🤷

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that PointSetBase should be wrapped with equivalent parameters to QuadEdgeMeshTraits. I guess it is not so right now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For every PointSet wrapping, there should be a PointSetBase wrapping. (But PointSetBase does not need as many wrappings as PointSet, because PointSetBase does not need to have a separate wrapping for each PixelType.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also in this case use ${ITKT_F} instead of float? But then, is ${ITKT_F} anything more than just an "abbreviation" for float?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-Dekker thanks for the follow-up.

Yes, this is likely related to the difference between the definition of itk::IdentifierType on Windows and other platforms. On Windows it resolves to unsigned long long to ensure 64-bit, on Unix unsigned long, which is already always 64-bit.

N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Oct 9, 2024
Aims to address Windows specific wrapping failures at the CI
(https://open.cdash.org/viewBuildError.php?type=1&buildid=9948358), including:

    itkQuadEdgeMeshBase: warning(4): ITK type not wrapped, or currently not known: itk::QuadEdgeMeshPoint< float, 2 >

And locally, from Visual Studio 2022:

    Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Nothing known about base class 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >'. Ignored.
    Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Maybe you forgot to instantiate 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >' using %template.

As mentioned at InsightSoftwareConsortium#4867 (comment)
dzenanz pushed a commit that referenced this pull request Oct 9, 2024
Aims to address Windows specific wrapping failures at the CI
(https://open.cdash.org/viewBuildError.php?type=1&buildid=9948358), including:

    itkQuadEdgeMeshBase: warning(4): ITK type not wrapped, or currently not known: itk::QuadEdgeMeshPoint< float, 2 >

And locally, from Visual Studio 2022:

    Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Nothing known about base class 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >'. Ignored.
    Bin\ITK\Wrapping\Typedefs\itkQuadEdgeMeshBase.i(1220): warning 401: Maybe you forgot to instantiate 'itk::PointSetBase< itk::MapContainer< unsigned long long,itk::QuadEdgeMeshPoint< float,2,itk::GeometricalQuadEdge< unsigned long long,unsigned long long,bool,bool > > > >' using %template.

As mentioned at #4867 (comment)
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Nov 14, 2024
- Anticipating to make use of `itk::PointSetBase`, which was introduced by pull request InsightSoftwareConsortium/ITK#4867 commit InsightSoftwareConsortium/ITK@bf0ed18 (October 7, 2024).
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jan 2, 2025
- Anticipating to make use of `itk::PointSetBase`, which was introduced by pull request InsightSoftwareConsortium/ITK#4867 commit InsightSoftwareConsortium/ITK@bf0ed18 (October 7, 2024).
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jan 7, 2025
- Anticipating to make use of `itk::PointSetBase`, which was introduced by pull request InsightSoftwareConsortium/ITK#4867 commit InsightSoftwareConsortium/ITK@bf0ed18 (October 7, 2024).

- Note that ITK v6.0a01 was problematic because of issue InsightSoftwareConsortium/ITK#4820 "5.4 ITK Failed to build(CMake) with error LNK2005: __ucrt_int_to_float already defined". A fix was included with v6.0a02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Python wrapping Python bindings for a class type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants