ComputeTriangleGeomCentroids: periodic centroid uses a constant-offset approximation (same defect class fixed in ComputeFeatureCentroids)

[imikejackson]

Summary

GeometryHelpers::Topology::AdjustCentroidsForPeriodicFaces(const BoundingBox3Df&, const BoundingBoxFaces&, ...) — used by ComputeTriangleGeomCentroids — computes a periodic (wrap-aware) centroid by adding a constant offset to the naive centroid:

// src/simplnx/Utilities/GeometryHelpers.cpp
centroids[3 * featureId + 0] += std::abs(maxPoint[0] - minPoint[0]) / 2.0f;  // and y, z

This is the same class of defect that was found and fixed for ComputeFeatureCentroids during its V&V (see the ImageGeom overload, now removed). A constant offset cannot compute a correct wrapped centroid: the true minimum-image centroid depends on where the feature's mass actually sits, not merely that the feature spans/touches opposing periodic faces. Two features that trip the same face condition but have different point distributions require different corrections, so no single offset is correct for both.

Why it's wrong

For a feature whose vertices wrap across a periodic boundary, the correct centroid is the minimum-image (circular) mean — unwrap the points at their largest empty gap (equivalently, average the unit vectors of their angular positions around the domain) and map the result back into the domain. Adding |max - min| / 2 instead lands the centroid in the empty middle of the wrapped feature, offset by an amount that is only coincidentally right for symmetric cases.

Note: unlike the ComputeFeatureCentroids ImageGeom overload (which also had a cell-units-vs-physical spacing bug), this BoundingBox overload works in physical coordinates, so it does not have a spacing bug — the flaw here is purely the constant-offset model.

Suggested fix

Apply the same treatment used in ComputeFeatureCentroids: replace the constant offset with a wrap-aware minimum-image / circular mean for any axis the feature spans, keeping the arithmetic mean for non-spanning axes and an arithmetic fallback for degenerate (domain-filling) features. For triangle geometry the per-feature vertex set is available, so the largest-gap method can be applied directly.

Reference

• Fixed analog: ComputeFeatureCentroids periodic path (reimplemented as circular mean; the wrong ImageGeom overload of AdjustCentroidsForPeriodicFaces was removed).
• Affected code: src/simplnx/Utilities/GeometryHelpers.cpp (AdjustCentroidsForPeriodicFaces BoundingBox overload) and its caller src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ComputeTriangleGeomCentroids.cpp.
• Affected users: only those enabling the Is Periodic option on ComputeTriangleGeomCentroids; the default is off.

Suggested scope

Fold this into a dedicated V&V of ComputeTriangleGeomCentroids (independent oracle for the periodic case, e.g. hand-derived minimum-image values on a small wrapping mesh), mirroring the ComputeFeatureCentroids V&V.

Surfaced by the ComputeFeatureCentroids V&V / adversarial review. Filed with Claude Code.

[imikejackson]

Verified the diagnosis against the code. The core claim holds: the constant |max - min| / 2 offset in the BoundingBox3Df overload of AdjustCentroidsForPeriodicFaces is only correct when a wrapping feature's mass is symmetric about the wrap. Concrete counterexample on a 1-D periodic domain [0, L] with 3 boundary vertices at x=0 and 1 at x=L: naive mean = L/4, + L/2 gives 3L/4, but the true minimum-image centroid is 0. Confirmed.

Two corrections/additions to the writeup:

1. State-of-tree accuracy. The claim that the ImageGeom overload "was removed" and that ComputeFeatureCentroids was "reimplemented as circular mean" is not yet true on develop. Both the ImageGeom overload (GeometryHelpers.cpp, xPoints/2.0f constant offset) and the constant-offset caller in ComputeFeatureCentroids.cpp are still present on develop; those changes live in the unmerged PR VV: Compute Feature Centroids full V&V'ed #1658. Recommend rewording the References section to point at VV: Compute Feature Centroids full V&V'ed #1658 as the in-progress analog rather than a landed fix.

2. Two flaws beyond the constant-offset model:

   • No wrap-back into the domain. centroids[...] += offset is unconditional with no modulo, so a naive centroid near the far face (e.g. 0.9L + L/2 = 1.4L) is reported outside the geometry's own bounding box. Any fix must map the result back into [minPoint, maxPoint].
   • Percolation vs. wrap is indistinguishable. A genuinely domain-spanning (percolating, non-wrapping) feature trips the same both-opposing-faces condition and gets the same erroneous shift. The fix needs a real wrap criterion (largest-gap test), not just a degenerate fallback.

Test coverage: ComputeTriangleGeomCentroidsTest.cpp never sets IsPeriodic, so the buggy branch has zero coverage — fully latent. The V&V oracle must include an asymmetric wrapping feature; a symmetric-only oracle would pass even the broken code.

No spacing-bug concern here is correct — the offset and centroids are both in physical vertex coordinates, so units match.

Starting a dedicated V&V of ComputeTriangleGeomCentroids per the suggested scope.

[imikejackson]

Correction to my previous comment — I was wrong about the state of PR #1658, and the error cuts against the premise of this issue, so worth stating plainly.

I claimed the circular-mean reimplementation and the removal of the ImageGeom overload "live in the unmerged PR #1658." They do not. Reading the actual vv/compute_feature_centroids branch:

• VV: Compute Feature Centroids full V&V'ed #1658 changes to AdjustCentroidsForPeriodicFaces are limited to the ImageGeom overload and fix only the spacing sub-bug (xPoints/2.0f → xPoints * spacing / 2.0f). The constant-offset model is retained.
• The ImageGeom overload is not removed, and nothing anywhere on the branch introduces a circular mean / minimum-image / largest-gap computation. The ComputeFeatureCentroids.cpp diff is a Kahan-variable rename + a count>0 cleanup, not an algorithm change.
• VV: Compute Feature Centroids full V&V'ed #1658's report explicitly states: "Sibling BoundingBox overload audited — sound (physical units), left unchanged." And its periodic fixture is validated "given the DREAM3D periodic-shift model (naive + (dim−1)/2)" — i.e. the constant-offset model is treated as correct-by-definition (a circular oracle), not checked against an independent centroid.

Consequences:

1. This issue's Reference section is inaccurate: the constant-offset defect was not fixed for ComputeFeatureCentroids. VV: Compute Feature Centroids full V&V'ed #1658 fixed a different sub-bug (spacing) and blessed the constant-offset model.
2. The same constant-offset defect still exists in ComputeFeatureCentroids (ImageGeom overload) on both develop and VV: Compute Feature Centroids full V&V'ed #1658 — an asymmetric feature that wraps the full extent gets an offset that is only coincidentally right for symmetric mass.
3. The periodic option is SIMPLNX-only in both filters (legacy FindFeatureCentroids / FindTriangleGeomCentroids have no periodic mode), so there is no legacy behavior to preserve — a mathematically-correct minimum-image centroid is the right target for both overloads.

The core defect this issue reports (constant offset in the BoundingBox overload) remains real and confirmed; only the "already fixed for the analog" framing was wrong. I'm proceeding with the ComputeTriangleGeomCentroids V&V and will establish the first independent periodic oracle for it. ComputeFeatureCentroids's identical unfixed defect is flagged separately for a decision on scope.

[imikejackson]

Fix is up in #1670.

Scope ended up broader than this issue as filed, because the review found the same defect in both overloads (not just the BoundingBox one this issue named):

• ComputeTriangleGeomCentroids — AdjustCentroidsForPeriodicFaces (BoundingBox overload) reimplemented as a largest-empty-gap minimum-image mean over the feature's vertices. Also fixed a latent std::bad_function_call in the algorithm (direct m_Callback invocation) that the periodic path triggered.
• ComputeFeatureCentroids — the ImageGeom overload carried the identical constant-offset defect (PR VV: Compute Feature Centroids full V&V'ed #1658 fixed only the spacing sub-bug and validated the model against itself). Reimplemented as a minimum-image circular mean accumulated during the cell sweep; the range-based ImageGeom overload was removed since per-feature index ranges cannot express a wrapped centroid.

Both are verified against independent Class 1 minimum-image oracles (the prior fixtures were circular), pass in-core and out-of-core, and the periodic option is SIMPLNX-only in both filters so no legacy behavior changes. The discriminating case: an asymmetric wrapped feature on a [0,4] domain where the old constant offset produced an out-of-bounds 4.625 and the correct minimum-image value is 3.625.

Leaving this issue open until #1670 merges.