Skip to content

COMP: Use modern macro for name of class #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dzenanz merged 9 commits into from
Mar 24, 2025
Merged

COMP: Use modern macro for name of class #58

dzenanz merged 9 commits into from
Mar 24, 2025

Conversation

@hjmjohnson
Copy link
Member

@hjmjohnson hjmjohnson commented Jan 25, 2025 •
edited by dzenanz
Loading

When preparing for the future with ITK by setting
ITK_FUTURE_LEGACY_REMOVE:BOOL=ON
ITK_LEGACY_REMOVEBOOL=ON

The future preferred macro should be used
│ - itkTypeMacro
│ + itkOverrideGetNameOfClassMacro

Closes #60.

@hjmjohnson hjmjohnson requested a review from dzenanz January 25, 2025 16:06
@hjmjohnson hjmjohnson self-assigned this Jan 25, 2025
@hjmjohnson hjmjohnson mentioned this pull request Jan 25, 2025
Closed
12 tasks
@hjmjohnson
Copy link
Member Author

hjmjohnson commented Jan 25, 2025

100% tests passed, 0 tests failed out of 18

@hjmjohnson hjmjohnson force-pushed the fix-legacy-remove branch 2 times, most recently from 63e0399 to d99010a Compare January 28, 2025 02:59
@hjmjohnson hjmjohnson force-pushed the fix-legacy-remove branch 2 times, most recently from 563480a to c275691 Compare March 9, 2025 15:52
@hjmjohnson
ENH: Update minimum cmake version to 3.16.3
b8cda52 
Match version for ITK v5.4.2
@hjmjohnson
COMP: Use modern macro for name of class
69b2742 
When preparing for the future with ITK by setting
ITK_FUTURE_LEGACY_REMOVE:BOOL=ON
ITK_LEGACY_REMOVEBOOL=ON

The future preferred macro should be used
│ -  itkTypeMacro
│ +  itkOverrideGetNameOfClassMacro
@hjmjohnson
STYLE: Update to match clang-format-19 from ITK
cb7cde3
@hjmjohnson
ENH: Use tag v5.4.2 for the build packages tags
6728241 
Set the default build package tags to v5.4.2
for capturing the ITKRemoteModuleBuildTestPackageAction
shared scripts.

This pulls the default configuration items needed
to build against ITK version v5.4.2.
@hjmjohnson
ENH: Update pyproject.toml for ITK 5.4.2
c4cd160 
In preparation for release.

Ensure that cmake is 3.16.3 or greater to
match v5.4.2 minimum requirements.

Set minimum python to 3.9

Increment version number.

Replace keyword of "ITK" with lowercase "itk"
in some instances to be consistent.
@hjmjohnson
ENH: Update actions/checkout to version 4.
78853e1
@hjmjohnson
Copy link
Member Author

hjmjohnson commented Mar 18, 2025

@thewtex

Failures like the following have been appearing recently. Could you provide a hint for resolving those issues?

Searching for wheels matching pattern dist/itk_*cp39*manylinux_2_28*aarch64.whl
ERROR    InvalidDistribution: Cannot find file (or expand pattern):             
         'dist/itk_*cp39*manylinux_2_28*aarch64.whl'                            
Error: Process completed with exit code 1.

@thewtex
Copy link
Member

thewtex commented Mar 18, 2025

@hjmjohnson the actual error can be seen in the earlier build step:

itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float, 3 >, double >

@thewtex
BUG: Ensure itkAnisotropicDiffusionLBRImageFilter wrapped first
3c4f1f1 
For:

itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not
wrapped, or currently not known:
itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short, 3 >, double
>
@hjmjohnson
Copy link
Member Author

hjmjohnson commented Mar 18, 2025

@hjmjohnson the actual error can be seen in the earlier build step:

@thewtex OH!!! I thought is was failing to download a file from the internet. I did not realize that the error was due to a local build issue. That is likely the case for other PR's as well!

Thank you!

@dzenanz
Copy link
Member

dzenanz commented Mar 18, 2025

That did not seem to have helped:

[19/27] cd /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/WorkingDirectory && /opt/python/cp310-cp310/bin/python /work/ITK-source/ITK/Wrapping/Generators/SwigInterface/igenerator.py --mdx /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs/AnisotropicDiffusionLBR.mdx --mdx /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs/ITKCommon.mdx --mdx /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs/ITKIOImageBase.mdx --mdx /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs/ITKIOSpatialObjects.mdx --mdx /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs/ITKImageGradient.mdx -w1 -w3 -w51 -w52 -w53 -w54 -A protected -A private -p /work/ITK-source/ITK/CMake/../Modules/ThirdParty/pygccxml/src -g /work/build/cp310-cp310-linux_aarch64/Wrapping/Generators/CastXML/castxml/bin/castxml --snake-case-file /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Generators/Python/itk/Configuration/AnisotropicDiffusionLBR_snake_case.py --interface-output-dir /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Typedefs --library-output-dir /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping --submodule-order "itkAnisotropicDiffusionLBRImageFilter;itkCoherenceEnhancingDiffusionImageFilter" --pyi_index_list "/work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Generators/Python/itk-pkl/itkAnisotropicDiffusionLBRImageFilter.index.txt;/work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Generators/Python/itk-pkl/itkCoherenceEnhancingDiffusionImageFilter.index.txt" --pyi_dir /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Generators/Python/itk --pkl_dir /work/ITK-cp310-cp310-manylinux_2_28_aarch64/Wrapping/Generators/Python/itk-pkl
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< double >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< double, 3 >, double >

@hjmjohnson
Copy link
Member Author

hjmjohnson commented Mar 21, 2025

@dzenanz I think the issue is that the second template option is set based on "RealType" of the pixel type of the first template argument. This second type is implied in our wrapping.

The warnings show up for pixel types where the default "RealType" is 32bit float instead of double.

I think that explicitly wrapping both template parameters explicitly might fix the problem, but I have not had time to figure out how to do this.

@dzenanz dzenanz requested a review from thewtex March 24, 2025 13:18
@dzenanz dzenanz mentioned this pull request Mar 24, 2025
Closed
@dzenanz
Copy link
Member

dzenanz commented Mar 24, 2025

This builds cleanly, and tests pass locally.

@dzenanz
Copy link
Member

dzenanz commented Mar 24, 2025

Mac tests fail due to ld: warning: ignoring duplicate libraries: '-lm'.

@dzenanz
Copy link
Member

dzenanz commented Mar 24, 2025

Let's see whether I have properly ignored the warning.

thewtex
thewtex requested changes Mar 24, 2025
View reviewed changes
Copy link
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.

Green CI, great! 🎉

A dependency on ITKIOPNG should not be added. From the linked issues, it seems a missing dependency in an example -- the dependency should be added to the example, not the module.

itk-module.cmake Outdated
ITKIOImageBase
ITKIOSpatialObjects
ITKMetaIO
ITKIOPNG
Copy link
Member

@thewtex thewtex Mar 24, 2025

Choose a reason for hiding this comment

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

This does not need ITKIOPNG.

Copy link
Member

@dzenanz dzenanz Mar 24, 2025

Choose a reason for hiding this comment

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

Example in this module already has ITKIOPNG in the list of required components:

ITKAnisotropicDiffusionLBR/examples/CMakeLists.txt

Line 9 in 513c881

ITKIOPNG

Neither built-in examples nor Sphinx Examples use ITKAnisotropicDiffusionLBR.

Maybe we should put both PNG and MetaIO in TEST_DEPENDS section? But we could discuss this in a separate PR (#59).

Copy link
Member

@thewtex thewtex Mar 24, 2025

Choose a reason for hiding this comment

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

I was referring to these examples:

https://github.com/InsightSoftwareConsortium/ITKAnisotropicDiffusionLBR/blob/main/examples/CMakeLists.txt

Maybe we should put both PNG and MetaIO in TEST_DEPENDS section? But we could discuss this in a separate PR (#59).

Yes, it could be addressed in another PR. But it should not be added in this PR.

@dzenanz
COMP: Fix warnings: ITK type not wrapped, or currently not known
f3f289e 
This commit fixes the following warnings:

itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned char, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< unsigned short, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< float, 3 >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< double >, double >
itkCoherenceEnhancingDiffusionImageFilter: warning(4): ITK type not wrapped, or currently not known: itk::AnisotropicDiffusionLBRImageFilter< itk::Image< double, 3 >, double >
@dzenanz dzenanz force-pushed the fix-legacy-remove branch from b89f221 to 7ff2942 Compare March 24, 2025 21:08
@dzenanz
Copy link
Member

dzenanz commented Mar 24, 2025

I split out contentious commit into a separate PR: #63.

thewtex
thewtex approved these changes Mar 24, 2025
View reviewed changes
Copy link
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.

@dzenanz thanks! 💯

@dzenanz dzenanz merged commit 4dbdfe9 into main Mar 24, 2025
3 checks passed
@thewtex thewtex deleted the fix-legacy-remove branch March 24, 2025 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dzenanz dzenanz dzenanz approved these changes

@thewtex thewtex thewtex approved these changes

Assignees

@hjmjohnson hjmjohnson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hjmjohnson @thewtex @dzenanz