Skip to content

STYLE: Replace ULL with IT as index type in itkPyVectorContainerTest.py - #6807

Open
N-Dekker wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:Replace-ULL-with-IT-in-itkPyVectorContainerTest
Open

STYLE: Replace ULL with IT as index type in itkPyVectorContainerTest.py#6807
N-Dekker wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:Replace-ULL-with-IT-in-itkPyVectorContainerTest

Conversation

@N-Dekker

Copy link
Copy Markdown
Contributor

Removed the check for sufficient wrapping from "Modules/Bridge/NumPy/wrapping/test/itkPyVectorContainerTest.py". itk.VectorContainer is always sufficiently wrapped for itk.IT.

Removed the check for sufficient wrapping from
"Modules/Bridge/NumPy/wrapping/test/itkPyVectorContainerTest.py".
`itk.VectorContainer` is always sufficiently wrapped for `itk.IT`.

Follow-up to pull request InsightSoftwareConsortium#6800
commit 0939d4c
"STYLE: Replace UL with IT as index type in itkPyVectorContainerTest.py",
(which was about the other "itkPyVectorContainerTest.py" file, in
"Filtering/MeshToPolyData/wrapping/test").
@github-actions github-actions Bot added area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Bridge Issues affecting the Bridge module type:Style Style changes: no logic impact (indentation, comments, naming) labels Aug 26, 2026

@dzenanz dzenanz left a comment

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.

Looks good on a glance.

@N-Dekker
N-Dekker marked this pull request as ready for review August 26, 2026 21:17
@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change switches the NumPy VectorContainer test to itk.IT indices and removes its availability check. The updated test now fails in supported reduced Python-wrapping builds that omit float wrapping or the 2D point specialization, where the previous test skipped successfully. Restore the availability guard or register the test only when its required float, 2D, and 3D templates are wrapped.

Confidence Score: 4/5

Not safe to merge until the test handles reduced wrapping configurations without failing.

A direct before-and-after execution reproduced missing-template failures when float wrapping is disabled and when 2D point wrapping is excluded.

Files Needing Attention: Modules/Bridge/NumPy/wrapping/test/itkPyVectorContainerTest.py needs an availability guard or equivalent conditional registration.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex published a finding-comment-proof for the P1 finding and attached a reproduction source and test-run logs to support review.
  • T-Rex published a second finding-comment-proof for the same P1 finding to document ongoing validation.
  • T-Rex documented a general-contract-validation-proof that explains how specific ITK wrap gates lead to KeyError and recommends a guard or conditional test registration.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Vector-container NumPy test no longer skips unavailable float/2D wrapping

    • Bug
      • The test is registered whenever Python wrapping is enabled, but it now unconditionally requests float scalar and 2D/3D point specializations. Builds with ITK_WRAP_float=OFF, or with dimension 2 excluded, fail the test instead of reporting insufficient wrapping and returning successfully.
    • Cause
      • The PR removed the runtime availability guard. The scalar IT,F specialization depends on WRAP_ITK_REAL (which excludes float when ITK_WRAP_float=OFF), while IT,P<F,d> is generated only for dimensions in ITK_WRAP_IMAGE_DIMS.
    • Fix
      • Restore a guard that verifies the required IT-indexed scalar and Point 2/3 vector-container templates, or gate registration in CMake on ITK_WRAP_float and both 2 and 3 being selected.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "STYLE: Replace ULL with IT as index type..." | Re-trigger Greptile

Comment on lines 35 to 85
@@ -78,7 +66,7 @@ def test_NumPyBridge_VectorContainer(self):
self.assertNotEqual(v2_cp.GetElement(0), arr_cp[0])

PointType = itk.Point[itk.F, 3]
v_point = itk.VectorContainer[itk.ULL, PointType].New()
v_point = itk.VectorContainer[itk.IT, PointType].New()
v_point.Reserve(2)
point = PointType()
point[0] = 1.0
@@ -95,7 +83,7 @@ def test_NumPyBridge_VectorContainer(self):
)

PointType = itk.Point[itk.F, 2]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Conditional wrappings go unchecked

The test is registered for every Python-wrapping build, but it now unconditionally requests the IT,F vector-container specialization and float Point vector containers in dimensions 2 and 3. Those templates are intentionally absent when ITK_WRAP_float=OFF or when either dimension is omitted from ITK_WRAP_IMAGE_DIMS, so these supported reduced configurations fail template lookup instead of skipping the test. Restore an equivalent availability guard, or only register this test when float plus dimensions 2 and 3 are wrapped.

Context Used: AGENTS.md (source)

Artifacts

Narrow vector-container wrapping reproduction source

  • The review-authored Python harness executes the parent and current test sources against the wrapping availability rules, with the takeaway that unavailable specializations are exercised directly.

Parent test run with float disabled and 2D excluded

  • The executed parent-source run prints its insufficient-wrapping skip and returns successfully for both reduced configurations, with the takeaway that the prior behavior was to skip.

Current test run with float disabled and 2D excluded

  • The executed current-source run reports missing `IT,F` when float is disabled and missing float Point 2D when dimension 2 is excluded, with the takeaway that the claimed regression is reproduced.

View artifacts

T-Rex Ran code and verified through T-Rex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Bridge Issues affecting the Bridge module area:Python wrapping Python bindings for a class type:Style Style changes: no logic impact (indentation, comments, naming) 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.

2 participants