Skip to content

BUG: Create orientation-space and pole-figure geometries without length units - #1838

Merged
imikejackson merged 4 commits into
BlueQuartzSoftware:developfrom
imikejackson:bug/orientation-geometry-length-units
Oct 26, 2026
Merged

imikejackson merged 4 commits into
BlueQuartzSoftware:developfrom
imikejackson:bug/orientation-geometry-length-units

Conversation

@imikejackson

Copy link
Copy Markdown
Contributor

Summary

Three Orientation Analysis filters created geometries with a physical length unit although their coordinates are not lengths:

Filter Geometry Coordinates Old unit
Convert Orientations To Rodrigues Geometry Vertex geometry Dimensionless orientation-space points Meter
Generate and Write Pole Figure Images Composed image and count image Pixel indices Micrometer
Compute GBCD Pole Figure Stereographic image on [-1,1] x [-1,1] Projection values Micrometer

All three now create their geometries with IGeometry::LengthUnit::Unspecified. The numeric coordinates are not changed.

  • Add an optional IGeometry::LengthUnit units argument to both CreateVertexGeometryAction constructors, in the same style as CreateImageGeometryAction. The default is the previous Meter value, so the other callers of the action are not changed. The unit is applied in apply(), is kept by clone(), and has a units() getter.
  • Make CreateImageGeometryAction::clone() pass its unit to the copy. The copy used the default Micrometer value before.
  • Add unit assertions to ConvertOrientationsToVertexGeometryTest (with and without fundamental-zone conversion), WritePoleFigureTest (composed image and count image), and ComputeGBCDPoleFigureTest. Before the fix these reported 10 == 100 (Meter) and 6 == 100 (Micrometer).
  • Add a test for CreateVertexGeometryAction that covers the four array handling modes, preflight and execute modes, direct and cloned application, and the default unit.
  • Add one or two short sentences to the three filter documentation pages that explain the unit choice.

Fixes #1803

Test Plan

  • New unit assertions fail before the fix and pass after it
  • ctest -R "ConvertOrientationsToVertexGeometry|WritePoleFigure|GBCDPoleFigure|CreateVertexGeometry|CreateImageGeometry|ImageGeom" passes (87 tests, serial run, macOS arm64 Release)
  • Full tree builds after the core action header change
  • clang-format 19.1.1 applied to the touched files

Comment thread src/simplnx/Filter/Actions/CreateVertexGeometryAction.hpp
Comment thread src/simplnx/Filter/Actions/CreateImageGeometryAction.cpp
Comment thread src/Plugins/OrientationAnalysis/test/ConvertOrientationsToVertexGeometryTest.cpp Outdated

@joeykleingers joeykleingers left a comment

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.

Fix the last finding and otherwise this looks good.

Comment thread src/simplnx/Filter/Actions/CreateVertexGeometryAction.hpp
@imikejackson
imikejackson force-pushed the bug/orientation-geometry-length-units branch from 0ea6035 to 028a6d7 Compare October 21, 2026 13:13
@imikejackson
imikejackson enabled auto-merge (squash) October 21, 2026 13:15
@imikejackson
imikejackson disabled auto-merge October 21, 2026 14:25
@imikejackson
imikejackson enabled auto-merge (squash) October 21, 2026 14:25

@joeykleingers joeykleingers left a comment

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.

Approving at ff75b2eb4. I pushed that commit to fix the last review findings:

  • CreateVertexGeometryAction checks the geometry it creates before it sets the units. When another DataObject already uses the path, it returns error -6108 with a fix hint instead of dereferencing a null geometry. A new GeometryTest case covers this in preflight and execute.
  • The new pole-figure unit checks use REQUIRE_NOTHROW on each geometry lookup first, and the new test pointers end in Ptr.

Verified locally on macOS arm64 (simplnx-Rel): the build has no warnings, and the 36 affected tests pass. Without the null check, the new test fails with SIGSEGV.

Problems that existed before this PR are not fixed here. I am filing or linking issues for them.

imikejackson and others added 4 commits October 24, 2026 08:49
…th units

* Convert Orientations To Rodrigues Geometry, Generate and Write Pole
  Figure Images, and Compute GBCD Pole Figure created geometries with
  the default Meter or Micrometer unit. Their coordinates are
  dimensionless orientation-space values, pixel indices, or
  stereographic projection values, not specimen lengths.
* Create these geometries with IGeometry::LengthUnit::Unspecified.
  The numeric coordinates do not change.
* Add an optional length-unit argument to CreateVertexGeometryAction.
  The default keeps the previous Meter behaviour for all other
  callers. The unit is applied in apply() and kept by clone().
* Make CreateImageGeometryAction::clone() carry the unit so a cloned
  action creates the same geometry as the original.
* Add unit assertions to the three filter tests and a test for the
  vertex action that covers every array handling mode, apply() with
  and without clone(), and the default unit.
* Document the unit choice in the three filter documentation pages.

Fixes BlueQuartzSoftware#1803

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
…e action cloning

Move the CreateVertexGeometryAction units test from the OrientationAnalysis
plugin into test/GeometryTest.cpp so the core action keeps coverage when the
plugin is disabled. Add a CreateImageGeometryAction test that applies a cloned
action and checks that the geometry keeps the requested length unit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nits default

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
…tighten the new tests

CreateVertexGeometryAction now checks the geometry it creates before it sets
the units, so a name collision returns an error instead of a crash. The tests
added by this change check each geometry lookup and follow the naming rules.
5 files changed, +72 / -39 lines.

**1. Vertex geometry creation failure**

- `src/simplnx/Filter/Actions/CreateVertexGeometryAction.hpp`
- `test/GeometryTest.cpp`

When VertexGeom::Create fails, for example because a DataArray already uses
the geometry path, the action returns error -6108. The message tells the user
to choose an unused path under a DataGroup or another geometry. Before, the
new setUnits call dereferenced the null geometry. A new test applies the
action where a DataArray already uses the path, in preflight and execute, and
checks the error code, the message and the existing array.

**2. Test checks and names**

- `src/Plugins/OrientationAnalysis/test/ComputeGBCDPoleFigureTest.cpp`
- `src/Plugins/OrientationAnalysis/test/WritePoleFigureTest.cpp`
- `src/Plugins/OrientationAnalysis/test/ConvertOrientationsToVertexGeometryTest.cpp`
- `test/GeometryTest.cpp`

Each new pole-figure unit check is preceded by REQUIRE_NOTHROW on the same
geometry lookup. Pointers added by the new tests end in Ptr, and the clone
flag reads as a condition.

Verification: simplnx-Rel (macOS arm64) builds with no warnings. The 36
affected tests pass: the action tests, ComputeGBCDPoleFigure, WritePoleFigure,
ConvertOrientationsToVertexGeometry, and the SimplnxCore filters that use
CreateVertexGeometryAction. Without the null check, the new collision test
fails with SIGSEGV. clang-format 19.1.1 reports no changes.
@imikejackson
imikejackson force-pushed the bug/orientation-geometry-length-units branch from 5af3871 to fbbdf29 Compare October 24, 2026 12:49
@imikejackson
imikejackson disabled auto-merge October 26, 2026 14:30
@imikejackson
imikejackson merged commit 31cfe81 into BlueQuartzSoftware:develop Oct 26, 2026
5 checks passed
@imikejackson
imikejackson deleted the bug/orientation-geometry-length-units branch October 26, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Use unspecified units for orientation and pole-figure outputs

2 participants