ENH: Standardize 2D Image Handling

[nyoungbq]

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

• 1 Unit test to test output from the filter against known exemplar set of data
• 1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

• No commented out code (rare exceptions to this is allowed..)
• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Updated API documentation (or API not changed)
• Added license to new files (if any)
• Added example pipelines that use the filter
• Classes and methods are properly documented

[imikejackson]

Code Review

What this PR does

Multi-purpose refactor of how SIMPLNX handles 2D/1D ImageGeometry cases (where one or more dimensions = 1):

1. New compile-time dimensionality dispatch in NeighborUtilities.hpp — a VoxelNeighbors<ImageDimensionState<...>> template provides per-shape (3D / Empty-X 2D / Empty-Y 2D / Empty-Z 2D / 1D / Single voxel) neighbor counts and offsets. ProcessCorners, ProcessEdges, ProcessFaces template helpers iterate the right boundary cells per shape via if constexpr branches.
2. Refactors ComputeFeatureNeighbors and IdentifySample to use the new dispatch — old hand-rolled Is1DImageDimsState/Is2DImageDimsState logic is replaced by templated helpers.
3. Removes the "fake spacing = 1.0" auto-fix in ImageGeom::findElementSizes and ComputeFeatureSizes::ProcessImageGeom. Users must now explicitly set spacing[empty_dim] = 1.0 (documented in Geometry.rst).
4. ComputeFeatureNeighbors doc note advising users to set spacing intentionally for 2D shared-surface-area math.
5. Modernizes loop control in IdentifySample: nested (z,y,x) loops with throttled progress messages and modulo-decoded indices.
6. API breakage: k_NegativeXNeighbor/etc. constants moved out of the nx::core namespace into VoxelNeighbors<T>:: static members. initializeFaceNeighborOffsets/initializeFaceNeighborInternalIdx/computeValidFaceNeighbors/computeFaceSurfaceAreas changed from returning std::array<…, 6> to returning std::vector<…>.

---

Critical Issues

• 🛑 Wrong stride values in initializeFaceNeighborOffsets for 2D cases — silent correctness bug. src/simplnx/Utilities/NeighborUtilities.hpp:244-269:

  if constexpr(std::is_same_v<ImageDimensionStateT, EmptyXImage2D>)
  {
    return {-dims[2], -1, 1, dims[2]};   // ❌ should be ±dims[1]
  }
  if constexpr(std::is_same_v<ImageDimensionStateT, EmptyYImage2D>)
  {
    return {-dims[2], -1, 1, dims[2]};   // ❌ should be ±dims[0]
  }
  if constexpr(std::is_same_v<ImageDimensionStateT, EmptyZImage2D>)
  {
    return {-dims[1], -1, 1, dims[1]};   // ❌ should be ±dims[0]
  }

  The stored linear index is always z*dims[0]*dims[1] + y*dims[0] + x. With dims[2]=1 (EmptyZImage2D), the Y-neighbor stride is dims[0], not dims[1]. Same problem in the other two empty-2D cases. These offsets are consumed by IdentifySample::operator() (line 36) and ComputeFeatureNeighbors::operator() (line 36) for any non-3D image. Square 2D images (dims[0] == dims[1]) hide the bug; non-square 2D images will compute wrong neighbors.

• 🛑 Self-include in NeighborUtilities.hpp — src/simplnx/Utilities/NeighborUtilities.hpp:3:

  #include "NeighborUtilities.hpp"   // self-include, almost certainly accidental

  #pragma once saves it from infinite recursion, but this is clearly a mistake — likely a typo for a different header.

• 🛑 Performance regression: std::array → std::vector in hot loops. Original computeValidFaceNeighbors, initializeFaceNeighborOffsets, initializeFaceNeighborInternalIdx returned stack-allocated std::array<…, 6>. They now return heap-allocated std::vector<…> (with std::vector<bool>'s packed-bit gymnastics for the flags). FillBadData::phaseFourIterativeFill calls computeValidFaceNeighbors twice per voxel inside an iterative outer loop — on a 512³ volume that's ~268M heap allocations per iteration. Recommended fix: keep std::array<…, MaxFaces> plus a count field, or take an output buffer by reference, or have the templated specializations return a fixed-size std::array whose size is VoxelNeighbors<T>::k_FaceNeighborCount (statically known). Lifting the call out of FillBadData's inner loop is also worth doing — the validity vector is recomputed twice on the same (xIdx,yIdx,zIdx).

• ⚠️ Precision loss in computeFaceSurfaceAreas — src/simplnx/Utilities/NeighborUtilities.hpp:322-326:

  std::vector<float64> computeFaceSurfaceAreas(...)
  {
    const auto zFace = static_cast<float32>(spacing[0] * spacing[1]);  // float64*float64 → float32
    ...
    return {zFace, ...};   // float32 implicitly widened back to float64
  }

  Returns float64 but truncates intermediates to float32. Drop the static_cast<float32> so the math stays in double precision.

---

Other Issues

• Inconsistent bounds-checking in IdentifySample — src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/IdentifySample.cpp:157-160:

  if(!checked.at(neighborPoint) && !goodVoxels.getValue(neighborPoint))
  {
    currentVList.push_back(neighborPoint);
    checked[neighborPoint] = true;

  .at() does bounds-checking and throws — every other access in the same loop uses []. Looks like a leftover debug aid. Switch to [] (or use .at() everywhere if genuinely needed).

• Breaking behavioral change in ImageGeom::findElementSizes — src/simplnx/DataStructure/Geometry/ImageGeom.cpp:160-164. Previous code coerced spacing of any empty dim to 1.0 implicitly and errored if 2+ dims were empty. Both protections removed:

  • 2D images that previously had stored z-spacing = 0.5 will compute element sizes 0.5× smaller than before.
  • 1D images (2 empty dims) now silently produce a wrong "volume" instead of erroring.

  The existing test had to change setSpacing(20.2f, 0.1f, 67777.1f) → setSpacing(20.2f, 0.1f, 1.0f) to keep passing. This is a silent break for any user with existing .dream3d files. Recommend either keeping the dimensional check that errors if >1 empty dim, or documenting this in the Image Geometry release notes more prominently than the python Geometry.rst.

• API break — global neighbor-index constants are gone. Free constants k_NegativeZNeighbor, ..., k_FaceNeighborCount removed from nx::core namespace, replaced by VoxelNeighbors<Image3D>::k_NegativeZNeighbor etc. Downstream plugins (SimplnxReview, FileStore, Synthetic) and DREAM3DNX were not updated in this PR. Anyone using these constants will hit a compile break. Confirm with downstream maintainers, or keep deprecated inline constexpr aliases at namespace scope.

• Naming/structure cruft from squashed iteration. ImageDimensionalUtilities sub-namespace was first added as a separate header (ImageDimensionalUtilities.hpp), then deleted, then re-added as a sub-namespace inside NeighborUtilities.hpp. Final state works, but consider renaming the inner namespace to something like nx::core::detail::VoxelTraversal. Also ProcessVoxels template is duplicated between ComputeFeatureNeighbors.cpp and IdentifySample.cpp — extract it.

• Documentation typos in wrapping/python/docs/source/Geometry.rst: dimesnional → dimensional, accomodate → accommodate, peice → piece, caluclation → calculation, evalute → evaluate, counter-intuitative → counter-intuitive.

• Use the detail::ImageDimensionality concept consistently. IdentifySample.cpp:13 declares template <typename ImageDimsStateT> with no constraint — could use the new concept like ComputeFeatureNeighbors.cpp:16.

---

Test Coverage

• Add hand-built 2D tests for IdentifySample and ComputeFeatureNeighbors covering each EmptyX/Y/Z dispatch with non-square dims (e.g. 3×4×1, 1×3×4). The current IdentifySampleTest.cpp only loads pre-generated exemplar .dream3d files and would not catch the wrong-stride bug noted above.
• Add a regression test for the ImageGeom::findElementSizes semantic change — at minimum, a test that confirms a 1D image (2 empty dims) errors or has a documented behavior.
• Add tests for ImageDimensionalUtilities::ProcessCorners/Edges/Faces themselves (e.g. recording which (x,y,z) indices the lambda is invoked with, for each dimensionality state).

---

Suggested merge order

1. Block on the wrong 2D strides + add 2D unit tests that prove the fix.
2. Trivial fixes: self-include, static_cast<float32>, .at() inconsistency.
3. Quantify the heap-allocation regression with one of the existing benchmarks; revert to std::array if it shows up.
4. Decide on the findElementSizes semantic break — at least add a release note.

Overall this is a worthwhile direction (compile-time dispatch over runtime branches is cleaner than the previous hand-rolled corner/edge enumeration), but the correctness of the EmptyXYZImage2D neighbor offsets and the API/semantic breakage need to be settled before this is safe to merge.

[imikejackson]

See comments