Repository navigation
BUG: C01: Copy-paste index drift in 15 filters #1861
Description
Summary
Some code was copied from a similar line, branch or code path, and one index, axis, array or variable was not changed in the copy. Examples are [0] instead of [1], meanArray instead of medianArray, bin instead of 2 * bin, and the Edge Attribute Matrix instead of the Face Attribute Matrix. The code compiles and usually runs without an error, so the result is wrong data.
As of develop 419f9db (2026-10-21) this class has 18 open findings in 15 filters: 0 CRASH, 16 WRONG_DATA, 2 MINOR. All findings are candidates. They were found and re-checked by reading the code, not by running it.
Mechanism
A filter often has parallel code: one block per axis, one branch per plane orientation (XY, YZ, XZ), a Direct and a Scanline algorithm, or a mean and a median path. When a block is copied, the reader expects every index in the copy to change. If one index stays the same, the copy reads or writes the wrong component, array or axis. Tests often use only the first case (an XY plane, one phase, the Direct path), so they do not see the error. In one finding the existing test even encodes the wrong value.
Example on develop. The median branch checks the mean pointer, which it copied from the mean branch:
if(inputValues->CalculateMedian)
{
medianArray = dataStructure.getDataRefAs<Float32Array>(inputValues->MedianPath).getDataStore();
if(meanArray == nullptr)
{
return MakeErrorResult(-69315, fmt::format("Median array from path {} invalid", inputValues->MedianPath.toString()));
}The same pattern was fixed in AlignGeometriesFilter by PR #1763 (8bed04c). The translation used component [0] three times; it now uses [0], [1] and [2]:
src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/AlignGeometries.cpp#L415
float translation[3] = {targetCentroid[0] - movingCentroid[0], targetCentroid[1] - movingCentroid[1], targetCentroid[2] - movingCentroid[2]};Rule
Each finding is a local error. No shared helper fixes the class. The rule for each fix:
- Make every parallel block use its own index, array or axis. Direct and Scanline must give the same result.
- For plane branches (YZ, XZ), map spacing, origin and centroid components from the same axis choice in all places.
- Prove the fix with an independent expected result (oracle). Do not use the output of the code under test.
Decision needed for two findings, because more than one correct output is possible:
- WriteAvizoUniformCoordinateFilter#2: (a) write the last lattice point
origin + res * (dims - 1), as the Rectilinear writer does, or (b) write the cell-center rangeorigin + 0.5 * restoorigin + (dims - 0.5) * res. Issue BUG: Avizo uniform export changes sample placement and spacing #1823 covers the same output; decide there. - SignedMaurerDistanceMapImageFilter#1: the value for an image with no boundary when Squared Distance is off. The proposal is
+sqrt(FLT_MAX), to match the squared branch and ITK.
How to fix
- Write the failing test first (see below).
- Change the one wrong index, array or variable. Look for the same copy in the sibling file (Direct and Scanline, or the 2D and 3D branch). Two findings already name both files: ComputeBoundingBoxStatsFilter#1 and QuickSurfaceMeshFilter#1.
- If an existing test expects the wrong value (ComputeArrayHistogramByFeatureFilter#1), change the expected value to the oracle value. Do not regenerate an exemplar file only to absorb the change; compute the expected value independently.
Ray.hpp(UncertainRegularGridSampleSurfaceMeshFilter#5) is shared code. Check every caller of the changed function and run their tests.- If a fix needs a new error code, use the filter's existing error-code range. Check that the code is unique with
git grep -nE -- "-NNNN([^0-9]|$)"on develop and on the open PR branches.
How to test
- Write the test first. It must fail on develop.
- Use the smallest input that shows the error: the trigger in the finding (for example a 1 x 20 x 20 Image Geometry for the YZ-plane findings, or a 2 x 2 x 2 Feature Ids block for QuickSurfaceMeshFilter#1).
- Compute the expected values by hand or with an independent tool, and put them in the test. Do not use the output of the filter as the expected value.
- For filters with Direct and Scanline algorithms, run the test for both with the existing algorithm-selection scope:
GENERATE(from_range(UnitTest::SelectAlgorithmTestScenariosForInMemoryStores()))andUnitTest::AlgorithmTestScope. - For plane findings, test all three plane orientations (XY, YZ, XZ) and check that the results are the same after the axis change.
Conventions
- One PR for the whole class, one commit per filter. Commit prefix
BUG:with aSigned-off-byline. No AI attribution. - Every fix starts with a unit test that fails on develop. If you cannot reproduce a finding, write that and the reason in the PR and on this issue. Do not force a fix.
- Value checks need an independent expected result (oracle), not the code's own output.
- Run ctest serially. Never use
-j; the tests depend on each other. - Format with clang-format 19.1.1 and the repository
.clang-format. - Do not add sentences about input requirements to the filter documentation. The generated parameter table documents them.
- If a fix touches a filter that has a V&V report (
src/Plugins/<Plugin>/vv/<Filter>.md), update every file of its V&V package (report, deviations, provenance, test tables). V&V documents reference DREAM3D 6.5.171 only. - Rebase on develop. Never merge develop into the branch.
Checklist
- WRONG_DATA · ComputeArrayHistogramByFeatureFilter · ComputeArrayHistogramByFeature.cpp:359 — Bin ranges are stored as lo/hi pairs at [2bin] and [2bin+1], but the modal ranges read [modalBin] and [modalBin+1] (Direct) and offset
feature*bins*2+bin(Scanline). The tests expect the wrong values.ComputeArrayHistogramByFeatureFilter#1 - WRONG_DATA · ComputeArrayStatisticsFilter · ComputeArrayStatistics.cpp:2094 — The grouped boolean mean compares the sum with the tuple count of the whole array, not with the group length. Mean and Std Dev per Feature are wrong.
ComputeArrayStatisticsFilter#3 - WRONG_DATA · ComputeBiasedFeaturesFilter · ComputeBiasedFeatures.cpp:320 — The box uses
centroidShift0/centroidShift1, but the classification reads centroid components 0 and 1. Features in YZ and XZ plane geometries are classified on the wrong axes.ComputeBiasedFeaturesFilter#1 - WRONG_DATA · ComputeBiasedFeaturesFilter · ComputeBiasedFeatures.cpp:263 — The upper bounds use spacing[0] and spacing[1] for every plane. A YZ or XZ plane with unequal spacing gets a wrong box.
ComputeBiasedFeaturesFilter#2 - WRONG_DATA · ComputeShapesFilter · ComputeShapes.cpp:432 — For a YZ or XZ plane (X or Y dimension 1), the 2D code still uses spacing[0]/[1], origin[0]/[1] and centroid components 0/1. Moments, axes and areas use the wrong axes.
ComputeShapesFilter#1 - WRONG_DATA · ComputeShapesFilter · ComputeShapes.cpp:495 — The 2D sub-points use the voxel corner (
xPoint * res + origin), but the centroids are cell centers. The points move by -res/2, so Ixy and the axis lengths and angles are wrong.ComputeShapesFilter#2 - WRONG_DATA · ComputeShapesFilter · ComputeShapes.cpp:755 — For Ixy == 0 and Ixx > Iyy the code writes
pi / 180.0F(1 degree) instead of pi/2.ComputeShapesFilter#6 - WRONG_DATA · ComputeShapesTriangleGeomFilter · ComputeShapesTriangleGeom.cpp:425 — The eigenvector matrix goes to
toEuler()with no handedness check. The Image Geometry version negates a row when the determinant is negative; here a left-handed set gives wrong Euler angles. (related: BUG: Preserve triangle shape axis lengths under rigid rotation #1802)ComputeShapesTriangleGeomFilter#5 - WRONG_DATA · ComputeSlipTransmissionMetricsFilter · ComputeSlipTransmissionMetrics.cpp:87 — The code compares the neighbor Phase index to 1 instead of checking that both phases are Cubic-High. Cubic pairs in phase 2 get 0.0 and a false -94739 warning; hexagonal phase-1 pairs are computed.
ComputeSlipTransmissionMetricsFilter#1 - WRONG_DATA · LaplacianSmoothingFilter · LaplacianSmoothing.cpp:32 — The code calls
setEdgeListId(0)before it reads the edge data id, so it reads 0. The Edge Attribute Matrix is orphaned, and an edge list that the input already had is deleted.LaplacianSmoothingFilter#3 - WRONG_DATA · QuickSurfaceMeshFilter · QuickSurfaceMeshDirect.cpp:473 — The problem-voxel test contains
f1 == f7 && f1 != f7, so it is never true and the v2/v7 diagonal case is never fixed (Direct line 473, Scanline line 845).QuickSurfaceMeshFilter#1 - WRONG_DATA · ReadDeformKeyFileV12Filter · ReadDeformKeyFileV12.cpp:505 — USRNOD data is written with
setTuple(0, data, tokens)for every user array. Each array gets token 0 at tuple 0 only, and execute reports success.ReadDeformKeyFileV12Filter#1 - WRONG_DATA · RemoveFlaggedTrianglesFilter · RemoveFlaggedTrianglesFilter.cpp:124 — The reduced Face Attribute Matrix name comes from
getEdgeAttributeMatrix(), notgetFaceAttributeMatrix(). The output Face Attribute Matrix gets a wrong name.RemoveFlaggedTrianglesFilter#3 - WRONG_DATA · SignedMaurerDistanceMapImageFilter · MaurerDistanceMapEngine.hpp:1814 — With Squared Distance off and no boundary in the image, the sqrt pass does not apply the +max fill that the squared branch applies. The output is -sqrt(FLT_MAX), which differs from Squared on and from ITK.
SignedMaurerDistanceMapImageFilter#1 - WRONG_DATA · UncertainRegularGridSampleSurfaceMeshFilter · Ray.hpp:227 — In the shared
Ray.hpp, the Y basis vector usescos1 * cos3where the ZXZ rotation needscos1 * sin3. The basis is not orthonormal and ray lengths change (0.69x to 1.73x).UncertainRegularGridSampleSurfaceMeshFilter#5 - WRONG_DATA · WriteAvizoUniformCoordinateFilter · WriteAvizoUniformCoordinate.cpp:69 — The BoundingBox maximum is
origin + res * dims. Avizo reads it as the last lattice point and infers spacingres * dims / (dims - 1). The Rectilinear writer usesdims - 1. (related: BUG: Avizo uniform export changes sample placement and spacing #1823)WriteAvizoUniformCoordinateFilter#2 - MINOR · ComputeBoundingBoxStatsFilter · ComputeBoundingBoxStatsDirect.cpp:1067 — After it gets the Median array, the code tests
meanArray == nullptr. With Calculate Median on and Calculate Mean off, execute fails with -69315 (alsoComputeBoundingBoxStatsScanline.cpp:877).ComputeBoundingBoxStatsFilter#1 - MINOR · ExecuteProcessFilter · ExecuteProcess.cpp:93 — Blocking mode sends stderr to
reproc::sink::null; non-blocking mode keeps it. With Should Block on, error output of the command is lost.ExecuteProcessFilter#5
Already fixed or in progress
Fixed on develop:
- BUG: Fix wrong output and invalid input handling in 19 filters #1765 — ReadImageStackFilter
ReadImageStackFilter#2 - BUG: Fix defects in seven SimplnxCore filters #1763 (8bed04c) — AlignGeometriesFilter
AlignGeometriesFilter#1, ComputeKMeansFilterComputeKMeansFilter#1 - BUG: Correct surface-area and sphericity calculations #1817 (01809c2) — ComputeSurfaceAreaToVolumeFilter
ComputeSurfaceAreaToVolumeFilter#1 - BUG: Fix hexahedron volumes and RectGrid partition spacing #1762 (11e2f02) — PartitionGeometryFilter
PartitionGeometryFilter#6
Related issues
- BUG: Preserve triangle shape axis lengths under rigid rotation #1802 "BUG: Preserve triangle shape axis lengths under rigid rotation" — partly covers ComputeShapesTriangleGeomFilter#5.
- BUG: Avizo uniform export changes sample placement and spacing #1823 "BUG: Avizo uniform export changes sample placement and spacing" — partly covers WriteAvizoUniformCoordinateFilter#2. Open PR BUG: Fix OOC Review Findings #1764 does not change this formula.
Out of scope
- No finding of this class is in a filter that has a V&V report (0 excluded).
- Bugs in the same filters that are not copy-paste errors belong to their own issues.
Activity
[-]BUG: Copy-paste index drift in 15 filters[/-][+]BUG: C01: Copy-paste index drift in 15 filters[/+]