Skip to content

BUG: AzimuthElevationToCartesianTransform::IsLinear() returns false backpoint to release-5.4 - #6796

Open
thewtex wants to merge 1 commit into
InsightSoftwareConsortium:release-5.4from
thewtex:azimuth-elevation-is-linear-release-5.4
Open

BUG: AzimuthElevationToCartesianTransform::IsLinear() returns false backpoint to release-5.4#6796
thewtex wants to merge 1 commit into
InsightSoftwareConsortium:release-5.4from
thewtex:azimuth-elevation-is-linear-release-5.4

Conversation

@thewtex

@thewtex thewtex commented Aug 25, 2026

Copy link
Copy Markdown
Member

The class derives from AffineTransform and overrides TransformPoint with a spherical to cartesian mapping, but inherited
MatrixOffsetTransformBase::IsLinear(), which returns true unconditionally. Code that reduces a transform to a matrix and an offset when IsLinear() is true silently produced a wrong matrix for this class.

Override IsLinear() to return false, consistent with the existing GetTransformCategory() override, and check both in the test, along with the mapping itself not scaling linearly.

Closes #6791

@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Aug 25, 2026
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change makes the azimuth/elevation transform report its nonlinear behavior and adds regression checks for both conversion directions. The added test currently cannot compile because it calls an unavailable itk::Math::Absolute API; replacing it with itk::Math::abs restores compilation.

Confidence Score: 4/5

Not merge-safe until the new test's unavailable math-function call is corrected.

There is one independent P1, non-security finding. A focused C++17 compilation reproduced the exact missing-member error and verified that the corresponding itk::Math::abs expression compiles, which maps to a score of 4.

Files Needing Attention: Modules/Core/Transform/test/itkAzimuthElevationToCartesianTransformTest.cxx

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P1 finding and linked it to the corresponding review comment.
  • T-Rex created a focused C++ compile validation script for the unavailable Absolute member and the supported abs member.
  • T-Rex captured compiler output for the test expression using itk Math Absolute and for the equivalent expression using itk Math abs.
  • T-Rex executed the paired focused C++ compile validation and produced an execution record for review.
  • The artifacts bundle was prepared to support reviewer verification of the validation steps.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "BUG: AzimuthElevationToCartesianTransfor..." | Re-trigger Greptile

Comment thread Modules/Core/Transform/test/itkAzimuthElevationToCartesianTransformTest.cxx Outdated
The class derives from AffineTransform and overrides TransformPoint with a
spherical to cartesian mapping, but inherited
MatrixOffsetTransformBase::IsLinear(), which returns true unconditionally.
Code that reduces a transform to a matrix and an offset when IsLinear() is
true silently produced a wrong matrix for this class.

Override IsLinear() to return false, consistent with the existing
GetTransformCategory() override, and check both in the test, along with
the mapping itself not scaling linearly.

Closes InsightSoftwareConsortium#6791
@hjmjohnson
hjmjohnson force-pushed the azimuth-elevation-is-linear-release-5.4 branch from 7fccb92 to 074ec1e Compare August 25, 2026 15:24
@hjmjohnson

Copy link
Copy Markdown
Member

Pushed a one-word compile fix to this branch so it isn't blocked overnight — itk::Math::Absoluteitk::Math::abs at itkAzimuthElevationToCartesianTransformTest.cxx:219. All six build failures were that single line.

itk::Math::Absolute was added after release-5.4 branched and does not exist here; the same test already used itk::Math::abs on lines 168-170, so this just makes line 219 consistent with the rest of the file. For a double argument the two are equivalent — Absolute's documented differences from abs concern integer and bool types.

@thewtex @valentin-boussot — I amended rather than adding a commit so the history stays one clean commit. Valentin remains the author; the committer field changed to me as a side effect of the amend. Force-pushed with --force-with-lease pinned to 7fccb92d4a0, so nothing of yours was overwritten. Happy to revert if you would rather carry the fix yourself.

The change
-      scalesLinearly &=
-        (itk::Math::Absolute(transformedDoubledPoint[i] - 2.0 * transformedPoint[i]) < ACCEPTABLE_ERROR);
+      scalesLinearly &= (itk::Math::abs(transformedDoubledPoint[i] - 2.0 * transformedPoint[i]) < ACCEPTABLE_ERROR);

clang-format rejoined the line because abs is short enough that the expression now fits inside 120 columns (116).

Verified locally on this branch

macOS 15 arm64, Apple clang via the pixi cxx environment, Release, BUILD_SHARED_LIBS=ON.

build: 1627/1627, 0 errors
1/1 Test #2614: itkAzimuthElevationToCartesianTransformTest ...   Passed
100% tests passed, 0 tests failed out of 1

The test passing matters beyond compilation: it asserts the mapping does not scale linearly in either direction, which is what justifies the IsLinear() override this PR adds.

I also checked the rest of the diff for other APIs that postdate release-5.4ITK_TEST_EXPECT_TRUE is present on this branch, and the IsLinear() override in the header uses nothing new. This was the only incompatibility.

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 type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances 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