From 733e5055e506b69c08c14e6fffe1447436ce525e Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Wed, 2 Sep 2026 13:04:27 -0400 Subject: [PATCH 1/3] VV: Remove/Extract Flagged Features fully V&V'ed Summary: - Found and fixed 5 bugs in the fill and extract paths (D2 fill loop never terminates when the input has FeatureId 0 cells, the vacated-cell test was `> 0` instead of `< 0`; D3 fill loop never terminates when no vacated cell has a fill source, now error -45436; D4 out-of-range FeatureId indexed the flag vector out of bounds, now error -45435 before any modification; D5 extract ignored sub-filter execute failures and threw on preflight failures, now errors -53901..-53904; D6 a flagged feature with no cells crashed the extract, now warning -53905 and skipped); D1, the first-sighting hang from issue #1698, was fixed in v7.4.2 by PR #1700 and gains its regression test; - documented 7 deviations from DREAM3D 6.5.171 (D1 first-sighting hang, D2 background-cell hang, D3 no-progress hang, D4 out-of-range FeatureId, D5 swallowed extract errors, D6 empty flagged feature on extract, D7 extracted geometry naming and carried feature Attribute Matrix); - retired 3 tests (fill path never exercised; extract checked only two values); - unit tests replaced with 15 inlined *Class 1 (Analytical) + Class 4 (Invariant)* test fixtures whose CellValue arrays prove the copy source of every filled cell, plus a Small IN100 invariant test that fills 8,535 cells; - added 3 V&V source-tree deliverables (report, deviations, provenance); - removed the duplicate NeighborList preflight warning (-11505; the shared -5558 warning remains), rewrote the filter documentation (fill semantics, background rule, error codes, deviation links), and added the v7.4.2 row to the release-dates table. Removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the extract geometry, origin and cell arrays match. Both hangs are reproduced by tests that spin forever on the previous code. Fixes #1698 Signed-off-by: Michael Jackson --- docs/dream3d_nx_release_dates.md | 1 + .../docs/RemoveFlaggedFeaturesFilter.md | 56 +- .../Algorithms/RemoveFlaggedFeatures.cpp | 133 +-- .../Filters/RemoveFlaggedFeaturesFilter.cpp | 15 - .../utils/FeatureRemovalUtilities.cpp | 102 +- .../utils/FeatureRemovalUtilities.hpp | 4 +- .../test/RemoveFlaggedFeaturesTest.cpp | 883 ++++++++++++++---- .../vv/RemoveFlaggedFeaturesFilter.md | 181 ++++ .../deviations/RemoveFlaggedFeaturesFilter.md | 133 +++ .../vv/provenance/6_5_test_data_1_v2.md | 3 +- 10 files changed, 1254 insertions(+), 257 deletions(-) create mode 100644 src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md create mode 100644 src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md diff --git a/docs/dream3d_nx_release_dates.md b/docs/dream3d_nx_release_dates.md index b3691cc793..8ff4786115 100644 --- a/docs/dream3d_nx_release_dates.md +++ b/docs/dream3d_nx_release_dates.md @@ -2,6 +2,7 @@ | Release Version | Release Date | |-----------------|--------------| +| 7.4.2 | 2026-08-31 | | 7.4.1 | 2026-03-23 | | 7.4.0 | 2025-10-27 | | 7.2.2 | 2025-09-28 | diff --git a/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md index 7f1797a62b..528dcac9f1 100644 --- a/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md @@ -6,21 +6,43 @@ Processing (Cleanup) ## Description -This **Filter** removes or extracts **Features** that have been flagged by an earlier filter in the pipeline. The user supplies a boolean array at the **Feature** level; features with a value of *false* are targeted for removal (or extraction). Removed features leave gaps in the cell-level **Feature Ids** array, which are then filled by [isotropic coarsening](RequireMinimumSizeFeaturesFilter.md) -- neighboring features grow outward uniformly until every cell is reassigned to a surviving feature. - -This filter is the general-purpose tool for discarding features based on any criterion you can express as a boolean flag (biased features, minority phases, user-selected grains, etc.). For the specific case of removing features smaller than a size threshold, see [Remove Minimum Size Features](RequireMinimumSizeFeaturesFilter.md) instead. +This **Filter** removes or extracts **Features** that an earlier filter in the pipeline has flagged. The user supplies a boolean (or unsigned 8-bit) array at the **Feature** level; features with a value of *true* are targeted for removal or extraction. This filter is the general-purpose tool for discarding features based on any criterion that can be expressed as a flag: biased features, surface features, minority phases, user-selected grains, and so on. For the specific case of removing features smaller than a size threshold, see [Remove Minimum Size Features](RequireMinimumSizeFeaturesFilter.md). For a rank-based selection ("remove the 10 smallest"), see [Keep/Remove Ranked Features](KeepRemoveRankedFeaturesFilter.md). ### Selected Operation The *Selected Operation* parameter provides the following choices: -- **Remove [0]**: Remove the flagged **Features** from the geometry. Neighboring features grow outward isotropically to fill the gaps. -- **Extract [1]**: Copy the flagged **Features** into a new separate geometry without modifying the original. -- **Extract then Remove [2]**: Copy the flagged **Features** into a new geometry and then remove them from the original (combining the previous two modes). +- **Remove [0]**: Remove the flagged **Features** from the geometry. Their **Cells** are set to 0 (background), or filled from neighboring features when *Fill-in Removed Features* is on. The feature **Attribute Matrix** is compacted and the surviving features are renumbered contiguously starting at 1. +- **Extract [1]**: Copy each flagged **Feature** into a new **Image Geometry** without modifying the original. +- **Extract then Remove [2]**: Extract first, from the unmodified data, and then remove. + +### How Fill-in Removed Features Works + +When *Fill-in Removed Features* is on, the vacated **Cells** are filled by isotropic coarsening: the neighboring features grow into the gap one layer per pass until no vacated cell remains. + +1. Every **Cell** of a flagged **Feature** is marked as vacated. +2. Each vacated cell polls its six face neighbors in the order -Z, -Y, -X, +X, +Y, +Z. Neighbors outside the volume and neighbors that are themselves vacated are ignored. +3. The cell copies every **Cell** level array (except those listed in *Attribute Arrays to Ignore*) from the neighbor whose feature is the most common among the polled neighbors. A tie goes to the feature seen first in the order above. +4. A vacated cell with no usable neighbor waits for the next pass, when its own neighbors have been filled. +5. Passes repeat until no vacated cell remains. + +Background cells (FeatureId 0) are never vacated and are never filled, but they do count as neighbors: a vacated cell whose only usable neighbors are background becomes background. This matches DREAM3D 6.5.171 and the other coarsening filters. + +If a pass cannot fill any remaining vacated cell, which happens only when every cell belonged to a flagged feature and the unflagged features own no cells, the filter stops with error *-45436* instead of running forever. + +### Extract Details + +Each flagged **Feature** is cropped along its axis-aligned bounding box (in cells) into a new **Image Geometry** named `-`. The id is zero-padded to the number of digits in the feature count so the geometries sort in order (for example *Extracted_Feature-03* when there are 12 features). The new geometry keeps the source spacing, has its origin at the bounding box corner, and carries every **Cell** array plus a copy of the feature **Attribute Matrix**. Features are not renumbered in the extracted geometry. + +A flagged **Feature** that owns no **Cells** has nothing to extract. The filter emits warning *-53905* for it and creates no geometry. + +### Input Validation + +Every value in *Cell Feature Ids* must be in the range 0 through (number of feature tuples - 1). A value outside that range stops the filter with error *-45435* before any data is modified. Flagging every **Feature** stops the filter with error *-45433*. ### WARNING: NeighborList Removal -When the operation is *Remove* or *Extract then Remove*, any *NeighborList* arrays in the Cell Feature **Attribute Matrix** will be **removed** because the neighbor relationships have changed. Re-run [Compute Feature Neighbors](ComputeFeatureNeighborsFilter.md) afterward to rebuild them. +When the operation is *Remove* or *Extract then Remove*, any *NeighborList* arrays in the feature **Attribute Matrix** are **removed** because the neighbor relationships have changed. Re-run [Compute Feature Neighbors](ComputeFeatureNeighborsFilter.md) afterward to rebuild them. ### Caveats @@ -29,12 +51,30 @@ This filter will **only** run on an **Image Geometry**. ### Required Input Sources - **Cell Feature Ids** -- produced by a segmentation filter such as [Segment Features (Misorientation)](../OrientationAnalysis/EBSDSegmentFeaturesFilter.md) or [Segment Features (Scalar)](ScalarSegmentFeaturesFilter.md). -- **Flag Array** -- a boolean feature-level array. Typical producers: [Compute Biased Features](ComputeBiasedFeaturesFilter.md), [Compute Surface Features](ComputeSurfaceFeaturesFilter.md), or a custom flag built via threshold/boolean operations on any feature-level statistic. +- **Flagged Features** -- a boolean feature-level array. Typical producers: [Compute Biased Features](ComputeBiasedFeaturesFilter.md), [Compute Surface Features](ComputeSurfaceFeaturesFilter.md), or a custom flag built with threshold or boolean operations on any feature-level statistic. % Auto generated parameter table will be inserted here ## Example Pipelines +## Differences from DREAM3D 6.5.171 + +For valid input, the removal and fill outputs match DREAM3D 6.5.171 exactly, and extracted geometries have the same dimensions, origin and cell data. SIMPLNX differs in how it handles malformed input and in how it packages extracted geometries: + +- [D3: Fill cannot make progress](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d3) -- SIMPLNX stops with error *-45436*; 6.5.171 runs forever. +- [D4: Feature ID outside the feature tuple range](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d4) -- SIMPLNX stops with error *-45435*; 6.5.171 reads out of bounds. +- [D6: Flagged feature with no cells on extract](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d6) -- SIMPLNX warns and skips it; 6.5.171 writes a spurious 1x1x1 geometry. +- [D7: Extracted geometry naming and contents](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d7) -- SIMPLNX uses `-` and carries the feature **Attribute Matrix**; 6.5.171 uses `Feature_` with cell data only. + +DREAM3D-NX releases before the fixes described in [D1](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d1) (v7.4.1 and earlier) and [D2](../vv/deviations/RemoveFlaggedFeaturesFilter.md#removeflaggedfeaturesfilter-d2) (v7.4.2 and earlier) could run forever with *Fill-in Removed Features* on. Upgrade or disable fill on those versions. + +## Related Filters + +- [Remove Minimum Size Features](RequireMinimumSizeFeaturesFilter.md) +- [Require Minimum Number of Neighbors](RequireMinNumNeighborsFilter.md) +- [Keep/Remove Ranked Features](KeepRemoveRankedFeaturesFilter.md) +- [Compute Feature Neighbors](ComputeFeatureNeighborsFilter.md) + ## License & Copyright Please see the description file distributed with this **Plugin** diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp index 7287187048..3847c28b5f 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp @@ -6,70 +6,63 @@ #include "simplnx/DataStructure/DataArray.hpp" #include "simplnx/Utilities/MaskCompareUtilities.hpp" -#include "simplnx/Utilities/ParallelTaskAlgorithm.hpp" using namespace nx::core; namespace { - -class RunCropImageGeometryImpl +constexpr int32 k_MaskArrayError = -53900; +constexpr int32 k_FeatureRectPreflightError = -53901; +constexpr int32 k_FeatureRectExecuteError = -53902; +constexpr int32 k_CropPreflightError = -53903; +constexpr int32 k_CropExecuteError = -53904; +constexpr int32 k_EmptyFeatureSkippedWarning = -53905; + +std::string FirstErrorMessage(const std::vector& errors) { -public: - RunCropImageGeometryImpl(DataStructure& dataStructure, const std::atomic_bool& shouldCancel, const DataPath& imageGeometryPath, const std::vector& minVoxelVector, - const std::vector& maxVoxelVector, const DataPath& createdImgGeomPath) - : m_DataStructure(dataStructure) - , m_ShouldCancel(shouldCancel) - , m_ImageGeometryPath(imageGeometryPath) - , m_MinVoxelVector(minVoxelVector) - , m_MaxVoxelVector(maxVoxelVector) - , m_CreatedImgGeomPath(createdImgGeomPath) - { - } - - ~RunCropImageGeometryImpl() = default; - - void operator()() const - { - CropImageGeometryFilter filter; + return errors.empty() ? std::string("(no error message)") : fmt::format("[{}] {}", errors[0].code, errors[0].message); +} - Arguments args; +/** + * @brief Crops one feature's bounding box out of the source Image Geometry into a new geometry. + */ +Result<> CropFeature(DataStructure& dataStructure, const std::atomic_bool& shouldCancel, const DataPath& imageGeometryPath, const std::vector& minVoxels, const std::vector& maxVoxels, + const DataPath& createdImgGeomPath) +{ + CropImageGeometryFilter filter; - args.insertOrAssign(CropImageGeometryFilter::k_RemoveOriginalGeometry_Key, std::make_any(false)); - args.insertOrAssign(CropImageGeometryFilter::k_SelectedImageGeometryPath_Key, std::make_any(m_ImageGeometryPath)); - args.insertOrAssign(CropImageGeometryFilter::k_RenumberFeatures_Key, std::make_any(false)); - args.insertOrAssign(CropImageGeometryFilter::k_UsePhysicalBounds_Key, std::make_any(false)); + Arguments args; - args.insertOrAssign(CropImageGeometryFilter::k_MinVoxel_Key, std::make_any>(m_MinVoxelVector)); - args.insertOrAssign(CropImageGeometryFilter::k_MaxVoxel_Key, std::make_any>(m_MaxVoxelVector)); - args.insertOrAssign(CropImageGeometryFilter::k_CreatedImageGeometryPath_Key, std::make_any(m_CreatedImgGeomPath)); + args.insertOrAssign(CropImageGeometryFilter::k_RemoveOriginalGeometry_Key, std::make_any(false)); + args.insertOrAssign(CropImageGeometryFilter::k_SelectedImageGeometryPath_Key, std::make_any(imageGeometryPath)); + args.insertOrAssign(CropImageGeometryFilter::k_RenumberFeatures_Key, std::make_any(false)); + args.insertOrAssign(CropImageGeometryFilter::k_UsePhysicalBounds_Key, std::make_any(false)); - auto preflightResult = filter.preflight(m_DataStructure, args); - if(preflightResult.outputActions.invalid()) - { - throw std::runtime_error("Preflight failed when cropping the geometry in extract flagged features!"); - } + args.insertOrAssign(CropImageGeometryFilter::k_MinVoxel_Key, std::make_any>(minVoxels)); + args.insertOrAssign(CropImageGeometryFilter::k_MaxVoxel_Key, std::make_any>(maxVoxels)); + args.insertOrAssign(CropImageGeometryFilter::k_CreatedImageGeometryPath_Key, std::make_any(createdImgGeomPath)); - if(m_ShouldCancel) - { - return; - } + auto preflightResult = filter.preflight(dataStructure, args); + if(preflightResult.outputActions.invalid()) + { + return MakeErrorResult(k_CropPreflightError, fmt::format("Preflight of the crop that extracts feature geometry '{}' (voxels [{}, {}, {}] to [{}, {}, {}]) from '{}' failed: {}", + createdImgGeomPath.toString(), minVoxels[0], minVoxels[1], minVoxels[2], maxVoxels[0], maxVoxels[1], maxVoxels[2], + imageGeometryPath.toString(), FirstErrorMessage(preflightResult.outputActions.errors()))); + } - auto executeResult = filter.execute(m_DataStructure, args); - if(preflightResult.outputActions.invalid()) - { - throw std::runtime_error("Execute failed when cropping the geometry in extract flagged features!"); - } + if(shouldCancel) + { + return {}; } -private: - DataStructure& m_DataStructure; - const std::atomic_bool& m_ShouldCancel; - const DataPath& m_ImageGeometryPath; - const std::vector& m_MinVoxelVector; - const std::vector& m_MaxVoxelVector; - const DataPath& m_CreatedImgGeomPath; -}; + auto executeResult = filter.execute(dataStructure, args); + if(executeResult.result.invalid()) + { + return MakeErrorResult(k_CropExecuteError, fmt::format("The crop that extracts feature geometry '{}' from '{}' failed: {}", createdImgGeomPath.toString(), imageGeometryPath.toString(), + FirstErrorMessage(executeResult.result.errors()))); + } + return {}; +} } // namespace // ----------------------------------------------------------------------------- @@ -105,7 +98,7 @@ Result<> RemoveFlaggedFeatures::operator()() // This really should NOT be happening as the path was verified during preflight BUT we may be calling this from // somewhere else that is NOT going through the normal nx::core::IFilter API of Preflight and Execute std::string message = fmt::format("Mask Array DataPath does not exist or is not of the correct type (Bool | UInt8) {}", m_InputValues->FlaggedFeaturesArrayPath.toString()); - return MakeErrorResult(-53900, message); + return MakeErrorResult(k_MaskArrayError, message); } if(getCancel()) @@ -113,6 +106,8 @@ Result<> RemoveFlaggedFeatures::operator()() return {}; } + Result<> result; + // Valid values Functionality::Extract and Functionality::ExtractThenRemove if(function != Functionality::Remove) { @@ -129,7 +124,8 @@ Result<> RemoveFlaggedFeatures::operator()() auto preflightResult = filter.preflight(m_DataStructure, args); if(preflightResult.outputActions.invalid()) { - throw std::runtime_error("Preflight failed when cropping the geometry in extract flagged features!"); + return MakeErrorResult(k_FeatureRectPreflightError, fmt::format("Preflight of the feature bounding-box computation for Feature Ids array '{}' failed: {}", + m_InputValues->FeatureIdsArrayPath.toString(), FirstErrorMessage(preflightResult.outputActions.errors()))); } if(getCancel()) @@ -138,9 +134,10 @@ Result<> RemoveFlaggedFeatures::operator()() } auto executeResult = filter.execute(m_DataStructure, args); - if(preflightResult.outputActions.invalid()) + if(executeResult.result.invalid()) { - throw std::runtime_error("Execute failed when cropping the geometry in extract flagged features!"); + return MakeErrorResult(k_FeatureRectExecuteError, fmt::format("The feature bounding-box computation for Feature Ids array '{}' failed: {}", m_InputValues->FeatureIdsArrayPath.toString(), + FirstErrorMessage(executeResult.result.errors()))); } } @@ -151,10 +148,8 @@ Result<> RemoveFlaggedFeatures::operator()() return {}; } - ParallelTaskAlgorithm taskRunner; - // This has to be run in serial for the time being because adding to the dataStructure is not thread-safe - taskRunner.setParallelizationEnabled(false); - + // Each crop adds a geometry to the DataStructure, which is not thread-safe, so the features are + // extracted one at a time. usize maxTuple = flaggedFeatures->getNumberOfTuples(); std::string paddingWidth = std::to_string(std::to_string(maxTuple).size()); for(usize i = 1; i < maxTuple; i++) @@ -173,12 +168,25 @@ Result<> RemoveFlaggedFeatures::operator()() std::vector minVoxels = {static_cast(bounds[index]), static_cast(bounds[index + 1]), static_cast(bounds[index + 2])}; std::vector maxVoxels = {static_cast(bounds[index + 3]), static_cast(bounds[index + 4]), static_cast(bounds[index + 5])}; + // ComputeFeatureRect initializes each minimum to the largest uint32 and each maximum to 0, so a + // feature that owns no cell is left with minimum > maximum. There is nothing to crop for it. + if(minVoxels[0] > maxVoxels[0] || minVoxels[1] > maxVoxels[1] || minVoxels[2] > maxVoxels[2]) + { + result.warnings().push_back( + Warning{k_EmptyFeatureSkippedWarning, fmt::format("Feature {} is flagged for extraction but owns no cell in the Feature Ids array '{}'. No geometry was created for it.", i, + m_InputValues->FeatureIdsArrayPath.toString())}); + continue; + } + DataPath createdImgGeomPath({fmt::format(fmt::runtime("{}-{:0" + paddingWidth + "d}"), m_InputValues->CreatedImageGeometryPrefix, i)}); m_MessageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("Now Extracting Feature {}", i)}); - taskRunner.execute(RunCropImageGeometryImpl(m_DataStructure, getCancel(), m_InputValues->ImageGeometryPath, minVoxels, maxVoxels, createdImgGeomPath)); + Result<> cropResult = CropFeature(m_DataStructure, getCancel(), m_InputValues->ImageGeometryPath, minVoxels, maxVoxels, createdImgGeomPath); + if(cropResult.invalid()) + { + return MergeResults(std::move(result), std::move(cropResult)); + } } - taskRunner.wait(); m_MessageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("All Features Successfully Extracted")}); } @@ -206,8 +214,9 @@ Result<> RemoveFlaggedFeatures::operator()() removalArgs.IgnoredDataArrayPaths = m_InputValues->IgnoredDataArrayPaths; removalArgs.FillRemovedFeatures = m_InputValues->FillRemovedFeatures; - return FeatureRemovalUtilities::removeFlaggedFeatures(m_DataStructure, flagVector, removalArgs, m_MessageHandler, m_ShouldCancel); + Result<> removeResult = FeatureRemovalUtilities::removeFlaggedFeatures(m_DataStructure, flagVector, removalArgs, m_MessageHandler, m_ShouldCancel); + return MergeResults(std::move(result), std::move(removeResult)); } - return {}; + return result; } diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp index 9189b6a575..f9ba982da8 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp @@ -5,7 +5,6 @@ #include "simplnx/DataStructure/AttributeMatrix.hpp" #include "simplnx/DataStructure/DataArray.hpp" #include "simplnx/DataStructure/DataPath.hpp" -#include "simplnx/DataStructure/INeighborList.hpp" #include "simplnx/Filter/Actions/CreateArrayAction.hpp" #include "simplnx/Filter/Actions/DeleteDataAction.hpp" #include "simplnx/Parameters/ArraySelectionParameter.hpp" @@ -131,20 +130,6 @@ IFilter::PreflightResult RemoveFlaggedFeaturesFilter::preflightImpl(const DataSt -9892, fmt::format("Could not find the parent Attribute Matrix for the selected Flagged Features Data Array at path '{}'", pFlaggedFeaturesArrayPathValue.toString()))}; } - std::string warningMsg; - for(const auto& [identifier, object] : *cellFeatureAmPtr) - { - if(const auto* srcNeighborListArrayPtr = dynamic_cast(object.get()); srcNeighborListArrayPtr != nullptr) - { - warningMsg += "\n" + cellFeatureAttributeMatrixPath.toString() + "/" + srcNeighborListArrayPtr->getName(); - } - } - if(!warningMsg.empty()) - { - resultOutputActions.m_Warnings.push_back(Warning({-11505, fmt::format("This filter modifies the Cell Level Array '{}', the following arrays are of type NeighborList and will not be kept:{}", - pFeatureIdsArrayPathValue.toString(), warningMsg)})); - } - auto pFunctionality = filterArgs.value(k_Functionality_Key); if(pFunctionality != to_underlying(Functionality::Remove)) { diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp index 8ca1656168..e4fa593b06 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp @@ -13,7 +13,35 @@ using namespace nx::core; namespace { -bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, std::vector& storageArray, const std::atomic_bool& shouldCancel, MessageHelper& messageHelper) +constexpr int32 k_AllFeaturesFlaggedError = -45433; +constexpr int32 k_RemoveInactiveObjectsError = -45434; +constexpr int32 k_FeatureIdOutOfRangeError = -45435; +constexpr int32 k_NoFillProgressError = -45436; + +/** + * @brief Outcome of one pass of IdentifyNeighbors() over the volume. + */ +struct NeighborScan +{ + /// At least one cell still holds a negative (vacated) FeatureId. + bool unresolvedCellsRemain = false; + /// At least one vacated cell has a non-negative face neighbor recorded as its fill source. + bool fillSourceFound = false; +}; + +/** + * @brief Chooses a fill source for every vacated cell. + * + * A vacated cell is one whose FeatureId is negative. Its six face neighbors are polled in the order + * -Z, -Y, -X, +X, +Y, +Z. Every non-negative neighbor FeatureId is tallied, background (0) included, + * and the source becomes the neighbor whose feature first reaches the highest tally. A cell with no + * non-negative neighbor gets no source this pass and is retried on the next pass, after its own + * neighbors have been filled. + * + * Cells with FeatureId 0 are background. They are never fill targets, so they do not keep the caller + * iterating. They are legal fill sources, which matches DREAM3D 6.5.171. + */ +NeighborScan IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, std::vector& storageArray, const std::atomic_bool& shouldCancel, MessageHelper& messageHelper) { ThrottledMessenger throttledMessenger = messageHelper.createThrottledMessenger(); @@ -29,7 +57,7 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, const std::array neighborVoxelIndexOffsets = initializeFaceNeighborOffsets(dims); constexpr std::array faceNeighborInternalIdx = initializeFaceNeighborInternalIdx(); - bool shouldLoop = false; + NeighborScan scan; auto progressIncrement = dims[2] / 100; usize progressCounter = 0; @@ -39,7 +67,7 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, { if(shouldCancel) { - return false; + return scan; } if(progressCounter > progressIncrement) @@ -57,11 +85,13 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, { int64 voxelIndex = kStride + jStride + xIdx; featureName = featureIds[voxelIndex]; - if(featureName > 0) + // Only vacated cells (negative) need a source. Background (0) is not a fill target; treating + // it as one would keep the caller's loop alive forever because nothing ever overwrites it. + if(featureName >= 0) { continue; } - shouldLoop = true; + scan.unresolvedCellsRemain = true; int32 current; int32 most = 0; std::vector numHits(6, 0); @@ -92,6 +122,7 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, { most = current; storageArray[voxelIndex] = static_cast(neighborPoint); + scan.fillSourceFound = true; } break; } @@ -100,14 +131,15 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, { // Count the first sighting as a hit. Without this the tally only records a neighbor on // the SECOND sighting of a feature, so a bad cell whose valid neighbors all belong to - // distinct features never gets a fill source. It stays negative, shouldLoop stays true, - // and the caller's do/while dilation loop never terminates. + // distinct features never gets a fill source. It stays negative, and the caller's + // do/while dilation loop never terminates. discoveredFeatures.push_back(feature); numHits[discoveredFeatures.size() - 1] = 1; if(1 > most) { most = 1; storageArray[voxelIndex] = static_cast(neighborPoint); + scan.fillSourceFound = true; } } } @@ -115,7 +147,30 @@ bool IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, } } } - return shouldLoop; + return scan; +} + +/** + * @brief Checks that every FeatureId indexes a tuple of the feature Attribute Matrix. + * + * FlagFeatures() indexes a vector of size totalFeatures with each cell's FeatureId, so a negative or + * too-large value is an out-of-bounds read. Run this before anything is modified. + */ +Result<> ValidateFeatureIds(const Int32AbstractDataStore& featureIds, usize totalFeatures, const DataPath& featureIdsPath, const DataPath& featureAttributeMatrixPath) +{ + const usize totalPoints = featureIds.getNumberOfTuples(); + for(usize i = 0; i < totalPoints; i++) + { + const int32 featureId = featureIds[i]; + if(featureId < 0 || static_cast(featureId) >= totalFeatures) + { + return MakeErrorResult(k_FeatureIdOutOfRangeError, + fmt::format("Cell {} of the Feature Ids array '{}' holds the value {}, but the feature Attribute Matrix '{}' has {} tuple(s), so the valid range is 0 through {}. Every " + "cell must reference a tuple of the feature Attribute Matrix. No data was modified.", + i, featureIdsPath.toString(), featureId, featureAttributeMatrixPath.toString(), totalFeatures, totalFeatures - 1)); + } + } + return {}; } std::vector FlagFeatures(Int32AbstractDataStore& featureIds, const std::vector& flaggedFeatures, const bool fillRemovedFeatures) @@ -199,10 +254,22 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector validation = ValidateFeatureIds(featureIds, flaggedFeatures.size(), args.FeatureIdsArrayPath, args.FeatureAttributeMatrixPath); + if(validation.invalid()) + { + return validation; + } + + if(shouldCancel) + { + return {}; + } + std::vector activeObjects = FlagFeatures(featureIds, flaggedFeatures, args.FillRemovedFeatures); if(activeObjects.empty()) { - return MakeErrorResult(-45433, "All Features were flagged and would all be removed. The filter has quit."); + return MakeErrorResult(k_AllFeaturesFlaggedError, fmt::format("All {} feature(s) in '{}' were flagged and would all be removed. At least one feature must remain. No data was modified.", + flaggedFeatures.size() - 1, args.FeatureAttributeMatrixPath.toString())); } if(shouldCancel) @@ -224,16 +291,29 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector> voxelArrays = GenerateDataArrayList(dataStructure, args.FeatureIdsArrayPath, args.IgnoredDataArrayPaths); FindVoxelArrays(featureIds, neighbors, voxelArrays, shouldCancel); + shouldLoop = scan.unresolvedCellsRemain; } while(shouldLoop); } @@ -245,7 +325,7 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector removeFlaggedFeatures(DataStructure& dataStructure, const std::vector& flaggedFeatures, const RemovalArgs& args, const IFilter::MessageHandler& messageHandler, const std::atomic_bool& shouldCancel); diff --git a/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp b/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp index f3b6bcb2cc..19322c8341 100644 --- a/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp +++ b/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp @@ -2,7 +2,9 @@ #include "SimplnxCore/SimplnxCore_test_dirs.hpp" #include "simplnx/Core/Application.hpp" +#include "simplnx/DataStructure/DataGroup.hpp" #include "simplnx/DataStructure/Geometry/ImageGeom.hpp" +#include "simplnx/DataStructure/NeighborList.hpp" #include "simplnx/Parameters/ArraySelectionParameter.hpp" #include "simplnx/Parameters/BoolParameter.hpp" #include "simplnx/Parameters/ChoicesParameter.hpp" @@ -12,237 +14,799 @@ #include "simplnx/UnitTest/UnitTestCommon.hpp" #include + +#include +#include #include #include +#include +#include +#include using namespace nx::core; using namespace nx::core::Constants; using namespace nx::core::UnitTest; namespace fs = std::filesystem; +/* + * V&V test suite for RemoveFlaggedFeaturesFilter. See src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md. + * + * Oracle: Class 1 (analytical) on hand-built fixtures, plus Class 4 (invariant) on Small IN100. + * + * Every fixture carries three Cell arrays so the fill pass can be checked cell-by-cell: + * FeatureIds - the segmentation under test. + * CellValue - 100 + cell index. Unique per cell, so after a fill the value in a vacated cell + * identifies exactly WHICH neighbor cell it was copied from. + * IgnoredValue - 500 + cell index. Passed as an ignored array and must never change. + * and one Feature array, Int32DataSet = 1000 + feature id, so the compaction can be checked tuple-by-tuple. + * + * Fill semantics under test (shared with RequireMinNumNeighbors and Keep/Remove Ranked Features via + * FeatureRemovalUtilities): every vacated cell (FeatureId -1) polls its six face neighbors in the order + * -Z, -Y, -X, +X, +Y, +Z, tallies each non-negative neighbor FeatureId (0 counts), and copies the Cell + * tuple of the neighbor whose feature reached the highest tally first. Ties resolve to the feature seen + * first in that order. Cells with no non-negative neighbor wait for the next iteration. + */ namespace { +constexpr StringLiteral k_CellValueName = "CellValue"; +constexpr StringLiteral k_IgnoredValueName = "IgnoredValue"; const std::string k_NewImgGeomPrefix = "NewImgGeom"; -const std::string k_NewImgGeom = k_NewImgGeomPrefix + "-3"; + const DataPath k_ImageGeomPath({k_DataContainer}); +const DataPath k_CellDataPath({k_DataContainer, k_CellData}); const DataPath k_FeatureIdsPath({k_DataContainer, k_CellData, k_FeatureIds}); +const DataPath k_CellValuePath({k_DataContainer, k_CellData, k_CellValueName}); +const DataPath k_IgnoredValuePath({k_DataContainer, k_CellData, k_IgnoredValueName}); +const DataPath k_FeatureAMPath({k_DataContainer, k_CellFeatureData}); const DataPath k_FlaggedFeaturesPath({k_DataContainer, k_CellFeatureData, k_ActiveName}); -const DataPath k_NewFeatureIdsPath({k_NewImgGeom, k_CellData, k_FeatureIds}); +const DataPath k_FeatureValuePath({k_DataContainer, k_CellFeatureData, k_Int32DataSet}); + +constexpr uint64 k_Remove = 0; +constexpr uint64 k_Extract = 1; +constexpr uint64 k_ExtractThenRemove = 2; -void FillDataStructure(DataStructure& dataStructure) +constexpr int32 k_AllFeaturesFlaggedError = -45433; +constexpr int32 k_FeatureIdOutOfRangeError = -45435; +constexpr int32 k_NoFillProgressError = -45436; +constexpr int32 k_ParentNotAttributeMatrixError = -9892; +constexpr int32 k_NeighborListRemovalWarning = -5558; +constexpr int32 k_EmptyFeatureSkippedWarning = -53905; + +struct FixtureSpec { - ImageGeom* imageGeom = ImageGeom::Create(dataStructure, k_DataContainer); - std::vector dims = {4, 4, 1}; - imageGeom->setDimensions(dims); - imageGeom->setOrigin(std::vector{0, 0, 0}); - imageGeom->setSpacing(std::vector{1, 1, 1}); - - std::vector tupleDims(dims.rbegin(), dims.rend()); - auto* attributeMatrix = AttributeMatrix::Create(dataStructure, k_CellData, tupleDims, imageGeom->getId()); - - imageGeom->setCellData(*attributeMatrix); - - Int32Array* featureIds = UnitTest::CreateTestDataArray(dataStructure, k_FeatureIds, tupleDims, {1}, attributeMatrix->getId()); - auto& testFeatIdsDataStore = featureIds->getDataStoreRef(); - testFeatIdsDataStore[0] = 0; - testFeatIdsDataStore[1] = 1; - testFeatIdsDataStore[2] = 1; - testFeatIdsDataStore[3] = 1; - testFeatIdsDataStore[4] = 1; - testFeatIdsDataStore[5] = 0; - testFeatIdsDataStore[6] = 2; - testFeatIdsDataStore[7] = 2; - testFeatIdsDataStore[8] = 2; - testFeatIdsDataStore[9] = 2; - testFeatIdsDataStore[10] = 0; - testFeatIdsDataStore[11] = 1; - testFeatIdsDataStore[12] = 2; - testFeatIdsDataStore[13] = 3; - testFeatIdsDataStore[14] = 3; - testFeatIdsDataStore[15] = 0; - - auto* featureAttributeMatrix = AttributeMatrix::Create(dataStructure, k_CellFeatureData, {4ULL}, imageGeom->getId()); - BoolArray* maskArray = BoolArray::CreateWithStore>(dataStructure, k_ActiveName, {4}, {1}, featureAttributeMatrix->getId()); - auto& maskDataStore = maskArray->getDataStoreRef(); - maskDataStore[0] = false; - maskDataStore[1] = false; - maskDataStore[2] = false; - maskDataStore[3] = true; - Int32Array* testArray = UnitTest::CreateTestDataArray(dataStructure, k_Int32DataSet, {4}, {1}, featureAttributeMatrix->getId()); - auto& testStore = testArray->getDataStoreRef(); - testStore[0] = 0; - testStore[1] = 4041; - testStore[2] = 10128; - testStore[3] = 2185; + SizeVec3 dims; // (x, y, z) + std::vector featureIds; // x fastest, then y, then z + usize numFeatures; // tuple count of the Feature Attribute Matrix (index 0 is the unused feature) + std::vector flagged; // size numFeatures; true means remove/extract +}; + +std::vector TupleDims(const SizeVec3& dims) +{ + return {dims[2], dims[1], dims[0]}; } -template -void ValidateResults(const Int32Array& featureIdsResult, const AttributeMatrix& cellFeatureAMResult, const Int32Array& testArrayResult) +/** + * @brief Builds an Image Geometry fixture. Every cell array is populated so that copies can be traced. + */ +void BuildFixture(DataStructure& dataStructure, const FixtureSpec& spec) { - REQUIRE(featureIdsResult[0] == 0); - REQUIRE(featureIdsResult[1] == 1); - REQUIRE(featureIdsResult[2] == 1); - REQUIRE(featureIdsResult[3] == 1); - REQUIRE(featureIdsResult[4] == 1); - REQUIRE(featureIdsResult[5] == 0); - REQUIRE(featureIdsResult[6] == 2); - REQUIRE(featureIdsResult[7] == 2); - REQUIRE(featureIdsResult[8] == 2); - REQUIRE(featureIdsResult[9] == 2); - REQUIRE(featureIdsResult[10] == 0); - REQUIRE(featureIdsResult[11] == 1); - REQUIRE(featureIdsResult[12] == 2); - if constexpr(RemoveV) + REQUIRE(spec.featureIds.size() == spec.dims[0] * spec.dims[1] * spec.dims[2]); + REQUIRE(spec.flagged.size() == spec.numFeatures); + + ImageGeom* imageGeom = ImageGeom::Create(dataStructure, k_DataContainer); + imageGeom->setDimensions(spec.dims); + imageGeom->setOrigin(FloatVec3{0.0f, 0.0f, 0.0f}); + imageGeom->setSpacing(FloatVec3{1.0f, 1.0f, 1.0f}); + + const std::vector tupleDims = TupleDims(spec.dims); + auto* cellAM = AttributeMatrix::Create(dataStructure, k_CellData, tupleDims, imageGeom->getId()); + imageGeom->setCellData(*cellAM); + + auto& featureIds = UnitTest::CreateTestDataArray(dataStructure, k_FeatureIds, tupleDims, {1}, cellAM->getId())->getDataStoreRef(); + auto& cellValue = UnitTest::CreateTestDataArray(dataStructure, k_CellValueName, tupleDims, {1}, cellAM->getId())->getDataStoreRef(); + auto& ignoredValue = UnitTest::CreateTestDataArray(dataStructure, k_IgnoredValueName, tupleDims, {1}, cellAM->getId())->getDataStoreRef(); + for(usize i = 0; i < spec.featureIds.size(); i++) { - REQUIRE(featureIdsResult[13] == 0); - REQUIRE(featureIdsResult[14] == 0); + featureIds[i] = spec.featureIds[i]; + cellValue[i] = static_cast(100 + i); + ignoredValue[i] = static_cast(500 + i); } - if constexpr(!RemoveV) + + auto* featureAM = AttributeMatrix::Create(dataStructure, k_CellFeatureData, {spec.numFeatures}, imageGeom->getId()); + auto& flags = BoolArray::CreateWithStore>(dataStructure, k_ActiveName, {spec.numFeatures}, {1}, featureAM->getId())->getDataStoreRef(); + auto& featureValue = UnitTest::CreateTestDataArray(dataStructure, k_Int32DataSet, {spec.numFeatures}, {1}, featureAM->getId())->getDataStoreRef(); + for(usize i = 0; i < spec.numFeatures; i++) { - REQUIRE(featureIdsResult[13] == 3); - REQUIRE(featureIdsResult[14] == 3); + flags[i] = spec.flagged[i]; + featureValue[i] = static_cast(1000 + i); } - REQUIRE(featureIdsResult[15] == 0); +} + +Arguments MakeArgs(uint64 functionality, bool fill, const std::vector& ignored = {k_IgnoredValuePath}) +{ + Arguments args; + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_Functionality_Key, std::make_any(functionality)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FillRemovedFeatures_Key, std::make_any(fill)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_SelectedImageGeometryPath_Key, std::make_any(k_ImageGeomPath)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CreatedImageGeometryPrefix_Key, std::make_any(k_NewImgGeomPrefix)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key, std::make_any(k_FeatureIdsPath)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(k_FlaggedFeaturesPath)); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_IgnoredDataArrayPaths_Key, std::make_any(ignored)); + return args; +} - if constexpr(RemoveV) +std::vector ReadInt32(const DataStructure& dataStructure, const DataPath& path) +{ + const auto& store = dataStructure.getDataRefAs(path).getDataStoreRef(); + std::vector values(store.getNumberOfTuples()); + for(usize i = 0; i < values.size(); i++) { - REQUIRE(cellFeatureAMResult.getNumberOfTuples() == 3); + values[i] = store[i]; } - if constexpr(!RemoveV) + return values; +} + +std::vector Sequence(int32 start, usize count) +{ + std::vector values(count); + std::iota(values.begin(), values.end(), start); + return values; +} + +/** + * @brief Runs preflight and execute, requiring both to be valid, and returns the execute result. + */ +IFilter::ExecuteResult RunValid(DataStructure& dataStructure, const Arguments& args) +{ + RemoveFlaggedFeaturesFilter filter; + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); + return executeResult; +} + +/** + * @brief Runs preflight (required valid) and execute (required invalid) and returns the first error code. + */ +int32 RunExecuteError(DataStructure& dataStructure, const Arguments& args) +{ + RemoveFlaggedFeaturesFilter filter; + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_INVALID(executeResult.result); + REQUIRE(!executeResult.result.errors().empty()); + return executeResult.result.errors()[0].code; +} + +bool HasWarningCode(const std::vector& warnings, int32 code) +{ + return std::any_of(warnings.begin(), warnings.end(), [code](const Warning& w) { return w.code == code; }); +} + +// --------------------------------------------------------------------------------------------------- +// Fixture A: the reproduction from GitHub issue #1698. 5x2x1, features 2 and 3 removed. +// +// y=0: 1 1 1 2 3 idx 0..4 +// y=1: 1 4 4 4 3 idx 5..9 +// +// Vacated cells 3, 4 and 9. Every valid neighbor of cell 3 is a distinct feature (1 at -X, 4 at +Y) +// and cell 9 has exactly one valid neighbor (4 at -X), so a tally that only records a source on the +// second sighting of a feature never fills either cell. +// --------------------------------------------------------------------------------------------------- +FixtureSpec FixtureA() +{ + return {SizeVec3{5, 2, 1}, {1, 1, 1, 2, 3, 1, 4, 4, 4, 3}, 5, {false, false, true, true, false}}; +} + +// --------------------------------------------------------------------------------------------------- +// Fixture B: 4x4x1 with background (FeatureId 0) cells. Feature 3 removed. +// +// y=0: 0 1 1 1 idx 0..3 +// y=1: 1 0 2 2 idx 4..7 +// y=2: 2 2 0 1 idx 8..11 +// y=3: 2 3 3 0 idx 12..15 +// +// Cell 14's only non-negative neighbors are background cells 10 (-Y) and 15 (+X), so it is filled FROM +// background and ends at 0. Background cells themselves are never fill targets. +// --------------------------------------------------------------------------------------------------- +FixtureSpec FixtureB() +{ + return {SizeVec3{4, 4, 1}, {0, 1, 1, 1, 1, 0, 2, 2, 2, 2, 0, 1, 2, 3, 3, 0}, 4, {false, false, false, true}}; +} + +// --------------------------------------------------------------------------------------------------- +// Fixture E: 3x3x3 exercising all six face directions, a 4-vs-2 majority vote where the minority +// feature is seen first, and a vacated cell (0,0,0) enclosed by other vacated cells so that it can +// only be filled on the second iteration. Features 3 and 4 removed. +// +// z=0: 4 4 1 / 4 2 1 / 1 1 1 idx 0..8 +// z=1: 4 1 1 / 2 3 2 / 1 1 1 idx 9..17 +// z=2: 1 1 1 / 1 2 1 / 1 1 1 idx 18..26 +// --------------------------------------------------------------------------------------------------- +FixtureSpec FixtureE() +{ + return {SizeVec3{3, 3, 3}, + { + 4, 4, 1, 4, 2, 1, 1, 1, 1, // z = 0 + 4, 1, 1, 2, 3, 2, 1, 1, 1, // z = 1 + 1, 1, 1, 1, 2, 1, 1, 1, 1, // z = 2 + }, + 5, + {false, false, false, true, true}}; +} + +/** + * @brief Checks that the Feature Attribute Matrix was compacted to exactly the expected surviving tuples, in order. + */ +void CheckFeatureArraysCompacted(const DataStructure& dataStructure, const std::vector& expectedFeatureValues) +{ + const auto& featureAM = dataStructure.getDataRefAs(k_FeatureAMPath); + REQUIRE(featureAM.getNumberOfTuples() == expectedFeatureValues.size()); + REQUIRE(ReadInt32(dataStructure, k_FeatureValuePath) == expectedFeatureValues); + const auto& flags = dataStructure.getDataRefAs(k_FlaggedFeaturesPath); + REQUIRE(flags.getNumberOfTuples() == expectedFeatureValues.size()); +} +} // namespace + +// ===================================================================================================== +// Class 1 analytical oracle: Remove +// ===================================================================================================== + +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Remove without fill", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + + SECTION("Fixture A (5x2x1)") { - REQUIRE(cellFeatureAMResult.getNumberOfTuples() == 4); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + RunValid(dataStructure, MakeArgs(k_Remove, false)); + + // Removed cells go to 0. Survivors 1 and 4 renumber to 1 and 2. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{1, 1, 1, 0, 0, 1, 2, 2, 2, 0}); + // No fill, so no Cell tuple is copied. + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == Sequence(100, 10)); + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == Sequence(500, 10)); + // Feature tuples 0, 1 and 4 are kept in order. + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1004}); + UnitTest::CheckArraysInheritTupleDims(dataStructure); } - REQUIRE(testArrayResult[0] == 0); - REQUIRE(testArrayResult[1] == 4041); - REQUIRE(testArrayResult[2] == 10128); - - if constexpr(!RemoveV) + SECTION("Fixture B (4x4x1 with background)") { - REQUIRE(testArrayResult[3] == 2185); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureB()); + RunValid(dataStructure, MakeArgs(k_Remove, false)); + + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{0, 1, 1, 1, 1, 0, 2, 2, 2, 2, 0, 1, 2, 0, 0, 0}); + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == Sequence(100, 16)); + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1002}); + UnitTest::CheckArraysInheritTupleDims(dataStructure); } } -void ValidateNewGeom(const Int32Array& featureIdsResult, const AttributeMatrix& cellFeatureAMResult, const Int32Array& testArrayResult) +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { - REQUIRE(featureIdsResult[0] == 3); - REQUIRE(featureIdsResult[1] == 3); + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + RunValid(dataStructure, MakeArgs(k_Remove, true)); + + // Iteration 1 (state 1 1 1 -1 -1 / 1 4 4 4 -1): + // cell 3: -X = cell 2 (feature 1, first sighting -> source), +X = cell 4 (-1), +Y = cell 8 (feature 4, tally 1, not > 1). + // -> feature 1 from cell 2. + // cell 4: -X = cell 3 (-1), +Y = cell 9 (-1). No source; waits. + // cell 9: -Y = cell 4 (-1), -X = cell 8 (feature 4). -> feature 4 from cell 8. + // Iteration 2 (state 1 1 1 1 -1 / 1 4 4 4 4): + // cell 4: -X = cell 3 (feature 1, first sighting -> source), +Y = cell 9 (feature 4, tie, loses). + // -> feature 1 from cell 3, whose CellValue is already the copy of cell 2 (102). + // Compaction: 1 -> 1, 4 -> 2. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{1, 1, 1, 1, 1, 1, 2, 2, 2, 2}); + + std::vector expectedCellValue = Sequence(100, 10); + expectedCellValue[3] = 102; + expectedCellValue[9] = 108; + expectedCellValue[4] = 102; + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == expectedCellValue); + + // The ignored array is never touched by the fill. + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == Sequence(500, 10)); + + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1004}); + UnitTest::CheckArraysInheritTupleDims(dataStructure); } -} // namespace -TEST_CASE("SimplnxCore::RemoveFlaggedFeatures: Test Remove Algorithm", "[SimplnxCore][RemoveFlaggedFeatures]") +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill copies every non-ignored Cell array", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { UnitTest::LoadPlugins(); - // Instantiate the filter, a DataStructure object and an Arguments Object - RemoveFlaggedFeaturesFilter filter; DataStructure dataStructure; - FillDataStructure(dataStructure); - Arguments args; + BuildFixture(dataStructure, FixtureA()); + // Empty ignore list: IgnoredValue must now follow the same copies as CellValue. + RunValid(dataStructure, MakeArgs(k_Remove, true, {})); + + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{1, 1, 1, 1, 1, 1, 2, 2, 2, 2}); + std::vector expectedIgnored = Sequence(500, 10); + expectedIgnored[3] = 502; + expectedIgnored[9] = 508; + expectedIgnored[4] = 502; + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == expectedIgnored); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} - // Create default Parameters for the filter. - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_Functionality_Key, std::make_any(0)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FillRemovedFeatures_Key, std::make_any(false)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_SelectedImageGeometryPath_Key, std::make_any(k_ImageGeomPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key, std::make_any(k_FeatureIdsPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(k_FlaggedFeaturesPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_IgnoredDataArrayPaths_Key, std::make_any(MultiArraySelectionParameter::ValueType{})); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill treats FeatureId 0 as a source and never a target", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureB()); + RunValid(dataStructure, MakeArgs(k_Remove, true)); + + // Iteration 1 (row y=3 is 2 -1 -1 0): + // cell 13: -Y = cell 9 (feature 2, tally 1 -> source 9), -X = cell 12 (feature 2, tally 2 -> source 12), +X = cell 14 (-1). + // -> feature 2 from cell 12. + // cell 14: -Y = cell 10 (feature 0, tally 1 -> source 10), -X = cell 13 (-1), +X = cell 15 (feature 0, tally 2 -> source 15). + // -> feature 0 from cell 15. Background is a legal fill source, matching DREAM3D 6.5.171. + // Iteration 2: no negative cells remain. The five background cells were never candidates. + // Compaction: 1 -> 1, 2 -> 2. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{0, 1, 1, 1, 1, 0, 2, 2, 2, 2, 0, 1, 2, 2, 0, 0}); + + std::vector expectedCellValue = Sequence(100, 16); + expectedCellValue[13] = 112; + expectedCellValue[14] = 115; + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == expectedCellValue); + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == Sequence(500, 16)); + + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1002}); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} - // Preflight the filter and check result - auto preflightResult = filter.preflight(dataStructure, args); - SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill majority vote and second iteration (3x3x3)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureE()); + RunValid(dataStructure, MakeArgs(k_Remove, true)); + + // Vacated cells: 0, 1, 3, 9 (feature 4) and 13 (feature 3). Neighbor order is -Z, -Y, -X, +X, +Y, +Z. + // Iteration 1: + // cell 0 (0,0,0): +X = 1 (-1), +Y = 3 (-1), +Z = 9 (-1). No source; waits. + // cell 1 (1,0,0): -X = 0 (-1), +X = 2 (f1, tally 1 -> src 2), +Y = 4 (f2, tally 1), +Z = 10 (f1, tally 2 -> src 10). + // -> feature 1 from cell 10. + // cell 3 (0,1,0): -Y = 0 (-1), +X = 4 (f2, tally 1 -> src 4), +Y = 6 (f1, tally 1), +Z = 12 (f2, tally 2 -> src 12). + // -> feature 2 from cell 12. + // cell 9 (0,0,1): -Z = 0 (-1), +X = 10 (f1, tally 1 -> src 10), +Y = 12 (f2, tally 1), +Z = 18 (f1, tally 2 -> src 18). + // -> feature 1 from cell 18. + // cell 13 (1,1,1): -Z = 4 (f2 -> src 4), -Y = 10 (f1, tally 1), -X = 12 (f2, tally 2 -> src 12), +X = 14 (f2, tally 3 -> src 14), + // +Y = 16 (f1, tally 2), +Z = 22 (f2, tally 4 -> src 22). + // -> feature 2 wins 4 to 2 from cell 22, even though feature 1 was in the running. + // Iteration 2: + // cell 0: +X = 1 (f1, tally 1 -> src 1), +Y = 3 (f2, tally 1), +Z = 9 (f1, tally 2 -> src 9). + // -> feature 1 from cell 9, whose CellValue is already the copy of cell 18 (118). + // Iteration 3: nothing left. Compaction: 1 -> 1, 2 -> 2. + const std::vector expectedFeatureIds = { + 1, 1, 1, 2, 2, 1, 1, 1, 1, // z = 0 + 1, 1, 1, 2, 2, 2, 1, 1, 1, // z = 1 + 1, 1, 1, 1, 2, 1, 1, 1, 1, // z = 2 + }; + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == expectedFeatureIds); + + std::vector expectedCellValue = Sequence(100, 27); + expectedCellValue[1] = 110; + expectedCellValue[3] = 112; + expectedCellValue[9] = 118; + expectedCellValue[13] = 122; + expectedCellValue[0] = 118; + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == expectedCellValue); + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == Sequence(500, 27)); + + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1002}); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} - // Execute the filter and check the result - auto executeResult = filter.execute(dataStructure, args); - SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); +// ===================================================================================================== +// Execute errors +// ===================================================================================================== + +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - all features flagged (-45433)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + for(const bool fill : {false, true}) + { + DYNAMIC_SECTION("Fill = " << fill) + { + DataStructure dataStructure; + FixtureSpec spec = FixtureA(); + std::fill(spec.flagged.begin() + 1, spec.flagged.end(), true); + BuildFixture(dataStructure, spec); + REQUIRE(RunExecuteError(dataStructure, MakeArgs(k_Remove, fill)) == k_AllFeaturesFlaggedError); + // Nothing was modified. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == FixtureA().featureIds); + CheckFeatureArraysCompacted(dataStructure, Sequence(1000, 5)); + } + } +} - const auto& featureIdsResult = dataStructure.getDataRefAs(k_FeatureIdsPath); - const auto& cellFeatureAMResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData})); - const auto& testArrayResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData, k_Int32DataSet})); - ValidateResults(featureIdsResult, cellFeatureAMResult, testArrayResult); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - FeatureId out of range (-45435)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); - UnitTest::CheckArraysInheritTupleDims(dataStructure); + SECTION("FeatureId equal to the feature tuple count") + { + DataStructure dataStructure; + FixtureSpec spec = FixtureA(); + spec.featureIds[6] = 5; // feature Attribute Matrix holds tuples 0..4 + BuildFixture(dataStructure, spec); + REQUIRE(RunExecuteError(dataStructure, MakeArgs(k_Remove, false)) == k_FeatureIdOutOfRangeError); + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == spec.featureIds); + } + + SECTION("Negative FeatureId") + { + DataStructure dataStructure; + FixtureSpec spec = FixtureA(); + spec.featureIds[0] = -1; + BuildFixture(dataStructure, spec); + REQUIRE(RunExecuteError(dataStructure, MakeArgs(k_Remove, true)) == k_FeatureIdOutOfRangeError); + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == spec.featureIds); + } } -TEST_CASE("SimplnxCore::RemoveFlaggedFeatures: Test Extract Algorithm", "[SimplnxCore][RemoveFlaggedFeatures]") +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - no fill progress (-45436)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { UnitTest::LoadPlugins(); - // Instantiate the filter, a DataStructure object and an Arguments Object - RemoveFlaggedFeaturesFilter filter; DataStructure dataStructure; - FillDataStructure(dataStructure); - Arguments args; + // Feature 2 owns no cells but is not flagged, so the all-flagged guard passes. Every cell belongs to + // the flagged feature 1, so after marking no cell has a non-negative face neighbor and the dilation + // can never make progress. + BuildFixture(dataStructure, {SizeVec3{4, 1, 1}, {1, 1, 1, 1}, 3, {false, true, false}}); + REQUIRE(RunExecuteError(dataStructure, MakeArgs(k_Remove, true)) == k_NoFillProgressError); + // The marking pass has already run; the error reports that the array was modified. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{-1, -1, -1, -1}); +} - // Create default Parameters for the filter. - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_Functionality_Key, std::make_any(1)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_SelectedImageGeometryPath_Key, std::make_any(k_ImageGeomPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CreatedImageGeometryPrefix_Key, std::make_any(k_NewImgGeomPrefix)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key, std::make_any(k_FeatureIdsPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(k_FlaggedFeaturesPath)); +// ===================================================================================================== +// Class 1 analytical oracle: Extract +// ===================================================================================================== - // Preflight the filter and check result - auto preflightResult = filter.preflight(dataStructure, args); - SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract crops the bounding box of each flagged feature", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureB()); + RunValid(dataStructure, MakeArgs(k_Extract, false)); + + // The original geometry is untouched by Extract. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == FixtureB().featureIds); + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == Sequence(100, 16)); + CheckFeatureArraysCompacted(dataStructure, Sequence(1000, 4)); + + // Feature 3 occupies cells 13 (x=1,y=3) and 14 (x=2,y=3): bounds x 1..2, y 3..3, z 0..0. + // Four feature tuples -> one digit of zero padding -> "NewImgGeom-3". + const DataPath newGeomPath({k_NewImgGeomPrefix + "-3"}); + const auto& newGeom = dataStructure.getDataRefAs(newGeomPath); + REQUIRE(newGeom.getDimensions() == SizeVec3{2, 1, 1}); + REQUIRE(newGeom.getOrigin() == FloatVec3{1.0f, 3.0f, 0.0f}); + REQUIRE(newGeom.getSpacing() == FloatVec3{1.0f, 1.0f, 1.0f}); + + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_FeatureIds)) == std::vector{3, 3}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_CellValueName)) == std::vector{113, 114}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_IgnoredValueName)) == std::vector{513, 514}); + + // The Feature Attribute Matrix is carried over unchanged (features are not renumbered). + const auto& newFeatureAM = dataStructure.getDataRefAs(newGeomPath.createChildPath(k_CellFeatureData)); + REQUIRE(newFeatureAM.getNumberOfTuples() == 4); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellFeatureData).createChildPath(k_Int32DataSet)) == Sequence(1000, 4)); + + // The temporary bounds array is deleted after execution. + REQUIRE(dataStructure.getData(k_FeatureAMPath.createChildPath("tempBounds")) == nullptr); - // Execute the filter and check the result - auto executeResult = filter.execute(dataStructure, args); - SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} -#ifdef SIMPLNX_WRITE_TEST_OUTPUT - WriteTestDataStructure(dataStructure, fmt::format("{}/extract_flagged_features.dream3d", unit_test::k_BinaryTestOutputDir)); -#endif +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract zero-pads names and handles several features", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + // 12x1x1 with cell i belonging to feature i (cell 0 is background). 12 feature tuples -> two digits. + std::vector flagged(12, false); + flagged[3] = true; + flagged[11] = true; + BuildFixture(dataStructure, {SizeVec3{12, 1, 1}, Sequence(0, 12), 12, flagged}); + RunValid(dataStructure, MakeArgs(k_Extract, false)); + + for(const int32 featureId : {3, 11}) + { + const DataPath newGeomPath({fmt::format("{}-{:02d}", k_NewImgGeomPrefix, featureId)}); + const auto& newGeom = dataStructure.getDataRefAs(newGeomPath); + REQUIRE(newGeom.getDimensions() == SizeVec3{1, 1, 1}); + REQUIRE(newGeom.getOrigin() == FloatVec3{static_cast(featureId), 0.0f, 0.0f}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_FeatureIds)) == std::vector{featureId}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_CellValueName)) == std::vector{100 + featureId}); + } + // Unflagged features produce no geometry. + REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-01"})) == nullptr); + REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-1"})) == nullptr); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} - auto& featureIdsResult = dataStructure.getDataRefAs(k_FeatureIdsPath); - auto& cellFeatureAMResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData})); - auto& testArrayResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData, k_Int32DataSet})); - ValidateResults(featureIdsResult, cellFeatureAMResult, testArrayResult); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Extract - flagged feature with no cells is skipped with a warning", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + // Feature 4 exists in the Feature Attribute Matrix but owns no cell. + FixtureSpec spec = FixtureB(); + spec.numFeatures = 5; + spec.flagged = {false, false, false, true, true}; + BuildFixture(dataStructure, spec); + auto executeResult = RunValid(dataStructure, MakeArgs(k_Extract, false)); + REQUIRE(HasWarningCode(executeResult.result.warnings(), k_EmptyFeatureSkippedWarning)); + + // Feature 3 is still extracted; feature 4 produces no geometry. + REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-3"})) != nullptr); + REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-4"})) == nullptr); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} - auto& newFeatureIdsResult = dataStructure.getDataRefAs(k_NewFeatureIdsPath); - auto& newCellFeatureAMResult = dataStructure.getDataRefAs(DataPath({k_NewImgGeom, k_CellFeatureData})); - auto& newTestArrayResult = dataStructure.getDataRefAs(DataPath({k_NewImgGeom, k_CellFeatureData, k_Int32DataSet})); - ValidateNewGeom(newFeatureIdsResult, newCellFeatureAMResult, newTestArrayResult); +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract then Remove with fill", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureB()); + RunValid(dataStructure, MakeArgs(k_ExtractThenRemove, true)); + + // Extraction happens first, on the unmodified data: same expectations as the Extract test. + const DataPath newGeomPath({k_NewImgGeomPrefix + "-3"}); + const auto& newGeom = dataStructure.getDataRefAs(newGeomPath); + REQUIRE(newGeom.getDimensions() == SizeVec3{2, 1, 1}); + REQUIRE(newGeom.getOrigin() == FloatVec3{1.0f, 3.0f, 0.0f}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_FeatureIds)) == std::vector{3, 3}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellData).createChildPath(k_CellValueName)) == std::vector{113, 114}); + REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellFeatureData).createChildPath(k_Int32DataSet)) == Sequence(1000, 4)); + + // Then removal with fill, on the original: same expectations as the FeatureId 0 fill test. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{0, 1, 1, 1, 1, 0, 2, 2, 2, 2, 0, 1, 2, 2, 0, 0}); + std::vector expectedCellValue = Sequence(100, 16); + expectedCellValue[13] = 112; + expectedCellValue[14] = 115; + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == expectedCellValue); + CheckFeatureArraysCompacted(dataStructure, {1000, 1001, 1002}); UnitTest::CheckArraysInheritTupleDims(dataStructure); } -TEST_CASE("SimplnxCore::RemoveFlaggedFeatures: Test Extract then Remove Algorithm", "[SimplnxCore][RemoveFlaggedFeatures]") +// ===================================================================================================== +// Preflight +// ===================================================================================================== + +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Error - flag array parent is not an Attribute Matrix (-9892)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { UnitTest::LoadPlugins(); - // Instantiate the filter, a DataStructure object and an Arguments Object - RemoveFlaggedFeaturesFilter filter; DataStructure dataStructure; - FillDataStructure(dataStructure); - Arguments args; + BuildFixture(dataStructure, FixtureA()); + // Put a second flag array in a plain DataGroup. + auto* group = DataGroup::Create(dataStructure, "FlagGroup"); + BoolArray::CreateWithStore>(dataStructure, k_ActiveName, {5}, {1}, group->getId()); - // Create default Parameters for the filter. - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_Functionality_Key, std::make_any(2)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FillRemovedFeatures_Key, std::make_any(false)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_SelectedImageGeometryPath_Key, std::make_any(k_ImageGeomPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CreatedImageGeometryPrefix_Key, std::make_any(k_NewImgGeomPrefix)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key, std::make_any(k_FeatureIdsPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(k_FlaggedFeaturesPath)); - args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_IgnoredDataArrayPaths_Key, std::make_any(MultiArraySelectionParameter::ValueType{})); + Arguments args = MakeArgs(k_Remove, false); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(DataPath({"FlagGroup", k_ActiveName}))); - // Preflight the filter and check result + RemoveFlaggedFeaturesFilter filter; auto preflightResult = filter.preflight(dataStructure, args); - SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions); + REQUIRE(preflightResult.outputActions.errors()[0].code == k_ParentNotAttributeMatrixError); +} + +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Warning - NeighborLists are removed (-5558)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + for(const uint64 functionality : {k_Remove, k_ExtractThenRemove}) + { + DYNAMIC_SECTION("Functionality = " << functionality) + { + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + const auto& featureAM = dataStructure.getDataRefAs(k_FeatureAMPath); + const DataPath neighborListPath = k_FeatureAMPath.createChildPath("NeighborList"); + Int32NeighborList::Create(dataStructure, "NeighborList", featureAM.getShape(), featureAM.getId()); + REQUIRE(dataStructure.getData(neighborListPath) != nullptr); + + RemoveFlaggedFeaturesFilter filter; + Arguments args = MakeArgs(functionality, false); + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + // Exactly one warning for the NeighborList: the filter must not report it twice. + REQUIRE(preflightResult.outputActions.warnings().size() == 1); + REQUIRE(preflightResult.outputActions.warnings()[0].code == k_NeighborListRemovalWarning); + + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); + REQUIRE(dataStructure.getData(neighborListPath) == nullptr); + } + } + + SECTION("Extract alone does not remove NeighborLists") + { + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + const auto& featureAM = dataStructure.getDataRefAs(k_FeatureAMPath); + const DataPath neighborListPath = k_FeatureAMPath.createChildPath("NeighborList"); + Int32NeighborList::Create(dataStructure, "NeighborList", featureAM.getShape(), featureAM.getId()); + + RemoveFlaggedFeaturesFilter filter; + Arguments args = MakeArgs(k_Extract, false); + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + REQUIRE(preflightResult.outputActions.warnings().empty()); + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); + REQUIRE(dataStructure.getData(neighborListPath) != nullptr); + } +} + +// ===================================================================================================== +// Class 4 invariants on Small IN100 +// ===================================================================================================== - // Execute the filter and check the result +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 4 Invariants - Small IN100 remove small features with fill", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + const nx::core::UnitTest::TestFileSentinel testDataSentinel(nx::core::unit_test::k_TestFilesDir, "6_5_test_data_1_v2.tar.gz", "6_5_test_data_1_v2"); + const auto baseDataFilePath = fs::path(fmt::format("{}/6_5_test_data_1_v2/6_5_test_data_1_v2.dream3d", nx::core::unit_test::k_TestFilesDir)); + DataStructure dataStructure = UnitTest::LoadDataStructure(baseDataFilePath); + + const DataPath featureIdsPath({k_DataContainer, k_CellData, k_FeatureIds}); + const DataPath cellPhasesPath({k_DataContainer, k_CellData, "Phases"}); + const DataPath featureAMPath({k_DataContainer, k_CellFeatureData}); + const DataPath numElementsPath = featureAMPath.createChildPath("NumElements"); + const DataPath featurePhasesPath = featureAMPath.createChildPath("Phases"); + const DataPath flagsPath = featureAMPath.createChildPath("SmallFeatures"); + const DataPath neighborListPath = featureAMPath.createChildPath("NeighborList"); + REQUIRE(dataStructure.getData(neighborListPath) != nullptr); + + // Snapshot the input. + const std::vector oldFeatureIds = ReadInt32(dataStructure, featureIdsPath); + const std::vector oldCellPhases = ReadInt32(dataStructure, cellPhasesPath); + const std::vector oldNumElements = ReadInt32(dataStructure, numElementsPath); + const std::vector oldFeaturePhases = ReadInt32(dataStructure, featurePhasesPath); + const usize numOldFeatures = oldNumElements.size(); + const auto& imageGeom = dataStructure.getDataRefAs(DataPath({k_DataContainer})); + const SizeVec3 dims = imageGeom.getDimensions(); + REQUIRE(oldFeatureIds.size() == dims[0] * dims[1] * dims[2]); + + // Input sanity: no background, every id in range, cell phase equals its feature's phase. + usize inputViolations = 0; + for(usize c = 0; c < oldFeatureIds.size(); c++) + { + if(oldFeatureIds[c] <= 0 || static_cast(oldFeatureIds[c]) >= numOldFeatures || oldCellPhases[c] != oldFeaturePhases[oldFeatureIds[c]]) + { + inputViolations++; + } + } + REQUIRE(inputViolations == 0); + + // Flag every feature with fewer than 100 cells (275 of 846 features on this data set). + constexpr int32 k_MinCells = 100; + const auto& featureAM = dataStructure.getDataRefAs(featureAMPath); + auto& flags = BoolArray::CreateWithStore>(dataStructure, "SmallFeatures", {numOldFeatures}, {1}, featureAM.getId())->getDataStoreRef(); + // The flag array itself is compacted by the filter, so keep an independent copy for the checks below. + std::vector flagged(numOldFeatures, false); + std::vector newNames(numOldFeatures, 0); + std::vector expectedNumElements = {oldNumElements[0]}; + std::vector expectedFeaturePhases = {oldFeaturePhases[0]}; + usize numFlagged = 0; + for(usize f = 1; f < numOldFeatures; f++) + { + flagged[f] = oldNumElements[f] < k_MinCells; + flags[f] = flagged[f]; + if(flagged[f]) + { + numFlagged++; + } + else + { + newNames[f] = static_cast(expectedNumElements.size()); + expectedNumElements.push_back(oldNumElements[f]); + expectedFeaturePhases.push_back(oldFeaturePhases[f]); + } + } + REQUIRE(numFlagged == 275); + const usize numSurvivors = expectedNumElements.size() - 1; + + Arguments args = MakeArgs(k_Remove, true, {}); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key, std::make_any(flagsPath)); + + RemoveFlaggedFeaturesFilter filter; + auto preflightResult = filter.preflight(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions); + REQUIRE(HasWarningCode(preflightResult.outputActions.warnings(), k_NeighborListRemovalWarning)); auto executeResult = filter.execute(dataStructure, args); SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); - const auto& featureIdsResult = dataStructure.getDataRefAs(k_FeatureIdsPath); - const auto& cellFeatureAMResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData})); - const auto& testArrayResult = dataStructure.getDataRefAs(DataPath({k_DataContainer, k_CellFeatureData, k_Int32DataSet})); - ValidateResults(featureIdsResult, cellFeatureAMResult, testArrayResult); - - auto& newFeatureIdsResult = dataStructure.getDataRefAs(k_NewFeatureIdsPath); - auto& newCellFeatureAMResult = dataStructure.getDataRefAs(DataPath({k_NewImgGeom, k_CellFeatureData})); - auto& newTestArrayResult = dataStructure.getDataRefAs(DataPath({k_NewImgGeom, k_CellFeatureData, k_Int32DataSet})); - ValidateNewGeom(newFeatureIdsResult, newCellFeatureAMResult, newTestArrayResult); + const std::vector newFeatureIds = ReadInt32(dataStructure, featureIdsPath); + const std::vector newCellPhases = ReadInt32(dataStructure, cellPhasesPath); + REQUIRE(newFeatureIds.size() == oldFeatureIds.size()); + + // I1: the Feature Attribute Matrix is compacted in order; feature-level arrays are copied, not recomputed. + REQUIRE(featureAM.getNumberOfTuples() == numSurvivors + 1); + REQUIRE(ReadInt32(dataStructure, numElementsPath) == expectedNumElements); + REQUIRE(ReadInt32(dataStructure, featurePhasesPath) == expectedFeaturePhases); + + // I2: NeighborLists are gone. + REQUIRE(dataStructure.getData(neighborListPath) == nullptr); + + // I3: every cell is assigned to a surviving feature; untouched cells follow the renumbering exactly. + // I4: a cell's phase still matches its (new) feature's phase, so the copied tuples are self-consistent. + // I5: every vacated cell shares its final id with at least one face neighbor (it was copied from one). + std::vector newCounts(numSurvivors + 1, 0); + usize numFilled = 0; + const auto neighborHasSameId = [&](usize c, int32 id) { + const usize z = c / (dims[0] * dims[1]); + const usize y = (c / dims[0]) % dims[1]; + const usize x = c % dims[0]; + const std::array, 6> candidates = {{ + {z > 0, -static_cast(dims[0] * dims[1])}, + {y > 0, -static_cast(dims[0])}, + {x > 0, -1}, + {x < dims[0] - 1, 1}, + {y < dims[1] - 1, static_cast(dims[0])}, + {z < dims[2] - 1, static_cast(dims[0] * dims[1])}, + }}; + return std::any_of(candidates.begin(), candidates.end(), [&](const auto& cand) { return cand.first && newFeatureIds[static_cast(static_cast(c) + cand.second)] == id; }); + }; + // One million cells: count violations and assert once per invariant so a failure stays readable. + usize outOfRange = 0; + usize phaseMismatch = 0; + usize filledWithoutMatchingNeighbor = 0; + usize untouchedRenumberedWrong = 0; + for(usize c = 0; c < newFeatureIds.size(); c++) + { + const int32 id = newFeatureIds[c]; + if(id <= 0 || static_cast(id) > numSurvivors) + { + outOfRange++; + continue; + } + newCounts[id]++; + if(newCellPhases[c] != expectedFeaturePhases[id]) + { + phaseMismatch++; + } + if(flagged[oldFeatureIds[c]]) + { + numFilled++; + if(!neighborHasSameId(c, id)) + { + filledWithoutMatchingNeighbor++; + } + } + else if(id != newNames[oldFeatureIds[c]]) + { + untouchedRenumberedWrong++; + } + } + REQUIRE(outOfRange == 0); + REQUIRE(phaseMismatch == 0); + REQUIRE(filledWithoutMatchingNeighbor == 0); + REQUIRE(untouchedRenumberedWrong == 0); + REQUIRE(numFilled == 8535); + + // I6: surviving features only grow, and the growth accounts for every vacated cell. + usize totalGrowth = 0; + for(usize id = 1; id <= numSurvivors; id++) + { + REQUIRE(newCounts[id] >= expectedNumElements[id]); + totalGrowth += static_cast(newCounts[id] - expectedNumElements[id]); + } + REQUIRE(totalGrowth == numFilled); UnitTest::CheckArraysInheritTupleDims(dataStructure); } +// ===================================================================================================== +// SIMPL conversion +// ===================================================================================================== + TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: SIMPL Backwards Compatibility", "[SimplnxCore][RemoveFlaggedFeaturesFilter][BackwardsCompatibility]") { auto app = Application::GetOrCreateInstance(); @@ -277,7 +841,8 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: SIMPL Backwards Compatibili const Arguments args = pipelineFilter->getArguments(); CHECK(args.value(RemoveFlaggedFeaturesFilter::k_FillRemovedFeatures_Key) == true); - // Complex type (MultiDataArraySelectionFilterParameterConverter) - verified by successful pipeline loading + CHECK(args.value(RemoveFlaggedFeaturesFilter::k_IgnoredDataArrayPaths_Key) == + MultiArraySelectionParameter::ValueType{DataPath({"DC", "AM", "DA1"}), DataPath({"DC", "AM", "DA2"})}); CHECK(args.value(RemoveFlaggedFeaturesFilter::k_SelectedImageGeometryPath_Key) == DataPath({"DataContainer"})); CHECK(args.value(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key) == DataPath({"DataContainer", "CellData", "TestArray"})); CHECK(args.value(RemoveFlaggedFeaturesFilter::k_FlaggedFeaturesArrayPath_Key) == DataPath({"DataContainer", "CellData", "TestArray"})); diff --git a/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md new file mode 100644 index 0000000000..a79574ec7e --- /dev/null +++ b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md @@ -0,0 +1,181 @@ +# V&V Report: RemoveFlaggedFeaturesFilter + +| | | +|-----------|------------------| +| Plugin | SimplnxCore | +| SIMPLNX UUID | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| DREAM3D 6.5.171 equivalent | Two legacy filters. *Remove* maps to `RemoveFlaggedFeatures` (SIMPL UUID `a8463056-3fa7-530b-847f-7f4cb78b8602`, `Source/Plugins/Processing/ProcessingFilters/RemoveFlaggedFeatures.{h,cpp}`). *Extract* maps to `ExtractFlaggedFeatures` (SIMPL UUID `e0555de5-bdc6-5bea-ba2f-aacfbec0a022`, `Source/Plugins/Sampling/SamplingFilters/ExtractFlaggedFeatures.{h,cpp}`). | +| Verified commit | ** | +| Status | READY FOR REVIEW | +| Sign-off | Michael A. Jackson , 2026-09-02 (author). Second engineer: pending second-engineer review. | + +## At a glance + +| Aspect | Current state | +|------------------------|--------------------------| +| Algorithm Relationship | **Minor changes** - SIMPLNX merges legacy `RemoveFlaggedFeatures` (Processing) and `ExtractFlaggedFeatures` (Sampling) into one filter with an operation selector. The removal and fill loop is a port that now lives in the shared `FeatureRemovalUtilities`; the extract path is rebuilt on the `ComputeFeatureRect` and `CropImageGeometry` sub-filters. | +| Oracle (confirmed) | **Class 1 analytical + Class 4 invariants** - three hand-derived fixtures (5x2x1, 4x4x1, 3x3x3) pin every filled cell to the exact neighbor it was copied from, plus a Small IN100 invariant test that fills 8,535 cells. Encoded in `test/RemoveFlaggedFeaturesTest.cpp`; all 16 test cases pass in the in-core and out-of-core builds. | +| Code paths enumerated | 20 of 28 paths are assertion-covered. The 8 uncovered paths are cancellation and defensive sub-filter failure paths. | +| Tests today | 16 test cases: 8 Class 1 analytical (remove, fill, extract, extract-then-remove), 3 execute-error, 1 preflight-error, 1 preflight-warning, 1 Class 4 Small IN100 invariant, 1 empty-feature warning, 1 SIMPL 6.4/6.5 conversion. The 3 original tests are retired. | +| Exemplar archive | No output archive. The inline oracle needs none. The Class 4 test reads the shared input archive `6_5_test_data_1_v2.tar.gz` (SHA512 `585b51ba...3027d6c`, sidecar exists). | +| Legacy comparison | **Run (2026-09-02)** - DREAM3D 6.5.171 and SIMPLNX were run on the 5x2x1, 4x4x1 and 3x3x3 fixtures with fill on and off, and on the 4x4x1 extract. Every removal array (25 array comparisons) matched the oracle and each other exactly. The extract geometry, origin and cell arrays matched. Seven deviations are documented; five are bugs, four of which were in SIMPLNX. | +| Bug flags | `RemoveFlaggedFeaturesFilter-D1` through `-D6`. D1 and D2 are hangs that only SIMPLNX had; D3 and D4 are shared with 6.5.171; D5 is SIMPLNX only; D6 affects both differently. All are fixed in SIMPLNX on this branch (D1 was fixed in v7.4.2). | +| V&V phase | Discovery, oracle, reconciliation, algorithm review, tests, dual-build pass, legacy comparison, deviations and documentation are complete. Outstanding: second-engineer review of the oracle and this report at PR review. | + +## Summary + +`RemoveFlaggedFeaturesFilter` removes or extracts the **Features** marked true in a boolean feature array, optionally filling vacated **Cells** by iterative dilation from face neighbors, then compacts the feature Attribute Matrix. Verification uses three Class 1 analytical fixtures whose per-cell copy sources are hand-derived, a Class 4 Small IN100 invariant test, error and preflight tests, and SIMPL conversion checks. The removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the V&V found and fixed two SIMPLNX-only infinite loops (background cells, and distinct first-sighting neighbors as reported in issue #1698), added guards for two failure modes shared with 6.5.171 (no fill progress, out-of-range FeatureId), and fixed the extract path's swallowed sub-filter errors and its crash on a flagged feature with no cells. + +## Algorithm Relationship + +*Classification:* Port | **Minor changes** | Rewrite | New filter + +*Evidence:* The SIMPLNX filter was introduced in PR #474 as a merge of the two legacy filters under one operation selector (*Remove*, *Extract*, *Extract then Remove*). The `Remove` branch follows legacy `RemoveFlaggedFeatures::remove_flaggedfeatures()` and `assign_badpoints()` step for step: mark, iteratively dilate, compact. PR #1700 moved that code into `FeatureRemovalUtilities::removeFlaggedFeatures()` so `KeepRemoveRankedFeaturesFilter` can share it. The `Extract` branch replaces legacy `ExtractFlaggedFeatures::find_feature_bounds()` plus its inline `CropImageGeometry` call with the `ComputeFeatureRectFilter` and `CropImageGeometryFilter` sub-filters. + +*Port-time deltas (removal path):* + +1. Face-neighbor bookkeeping uses the shared `NeighborUtilities` helpers instead of six inline boundary tests. The traversal order (-Z, -Y, -X, +X, +Y, +Z) and the boundary rules are the same, so the output is unchanged. Confirmed by the bit-identical A/B. +2. The neighbor tally is a per-cell `discoveredFeatures`/`numHits` pair instead of the legacy feature-indexed `n[]` vector. Before PR #1700 the SIMPLNX tally did not count the first sighting, which changed output by never filling some cells (D1). With the first sighting counted, the tally is equivalent to legacy. +3. The vacated-cell test was `featureName > 0` instead of the legacy `featurename < 0`, which made background cells (FeatureId 0) permanent loop targets (D2). This branch restores `>= 0 -> skip`, matching legacy. +4. Two guards with no legacy counterpart were added on this branch: FeatureId range validation before any modification (D4) and a no-progress check on the fill loop (D3). Both turn undefined behavior or an infinite loop into a deterministic error and do not change valid output. +5. The per-cell neighbor buffer is allocated only when fill is enabled (PR #1700). No output effect. +6. Progress messages are throttled (PR #1340). No output effect. + +*Port-time deltas (extract path):* + +7. Bounds come from `ComputeFeatureRectFilter` (min initialized to `UINT32_MAX`, max to 0) instead of the legacy `-1` sentinel. For a flagged feature with no cells the legacy filter crops a bogus 1x1x1 geometry at origin (-1, -1, -1) (D6); SIMPLNX now warns and skips the feature. +8. The extracted geometry is named `-` and carries the feature Attribute Matrix. Legacy names it `Feature_` and copies only cell data (D7). +9. Sub-filter preflight and execute results were tested against the wrong variable and failures threw `std::runtime_error` (D5). This branch returns error results `-53901` through `-53904`. + +*Material PRs since the filter was introduced:* #474 (introduced), #926 (NeighborList removal warning), #1017 (DataStore), #1278 (FeatureId range checks elsewhere), #1523 (shared face-neighbor helpers), #1590 (2D image handling), #1700 (shared `FeatureRemovalUtilities`, first-sighting fix). See the deviation entries for which of these changed output. + +## Oracle + +*Class:* **1 (Analytical)** primary + **4 (Invariant)** companion. + +Expected values are derived by hand from the fixture definitions and the documented face-neighbor rule, without using output from SIMPLNX or DREAM3D 6.5.171. + +*Applied:* Every fixture carries three cell arrays: `FeatureIds`, `CellValue = 100 + cell index`, and `IgnoredValue = 500 + cell index`, plus a feature array `Int32DataSet = 1000 + feature id`. Because `CellValue` is unique per cell, the value found in a vacated cell after the fill identifies exactly which neighbor it was copied from, and copies that chain across iterations are visible. The derivations are written as comments beside each `REQUIRE` in the test. + +| Fixture | Geometry | What it pins | +|---|---|---| +| A (issue #1698) | 5x2x1, remove features 2 and 3 | A vacated cell whose valid neighbors are all distinct features (first sighting must count), a cell with one valid neighbor, a cell with no valid neighbor in pass 1 that is filled in pass 2, a 1-vs-1 tie resolved to the first-seen feature, and a two-step copy chain (cell 4 receives cell 2's value through cell 3). Expected `FeatureIds = [1,1,1,1,1, 1,2,2,2,2]`. | +| B (background) | 4x4x1 with five FeatureId 0 cells, remove feature 3 | Background cells are never fill targets and are legal fill sources: cell 14 is filled from background cell 15 and ends at 0. Expected `FeatureIds = [0,1,1,1, 1,0,2,2, 2,2,0,1, 2,2,0,0]`. Also the extract fixture: feature 3 crops to a 2x1x1 geometry at origin (1, 3, 0). | +| E (3D) | 3x3x3, remove features 3 and 4 | All six face directions, a 4-vs-2 majority where the losing feature is seen first (cell 13 copies from +Z cell 22), and corner cell 0 enclosed by vacated cells so that it is filled only in pass 2 from cell 9, which itself was filled from cell 18. | +| Small IN100 | 100x100x100, remove 275 features with fewer than 100 cells, fill on | Class 4: every cell ends in a surviving feature, untouched cells follow the compaction renumbering exactly, feature arrays are copied not recomputed, each cell's phase equals its new feature's phase, every one of the 8,535 filled cells shares its final id with a face neighbor, and survivors grow by exactly the number of filled cells. NeighborLists are removed. | + +*Encoded:* `src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp` - `Class 1 Oracle - *` (8 test cases) and `Class 4 Invariants - Small IN100 remove small features with fill`. All pass at the verified commit in the `simplnx-Rel` and `simplnx-ooc-Rel` builds. + +*Second-engineer review:* pending second-engineer review (the PR review constitutes the review). + +## Bugs found and fixed + +*This branch fixes every defect in this table except D1, which PR #1700 fixed in DREAM3D-NX v7.4.2 before this V&V; this branch adds its regression test. The remaining fixes will be in the DREAM3D-NX release after v7.4.2.* + +| Deviation | Defect | Affected released versions | Resolution in this branch | +|-----------|--------|----------------------------|---------------------------| +| `RemoveFlaggedFeaturesFilter-D1` | With *Fill-in Removed Features* on, the neighbor tally only recorded a fill source on the second sighting of a feature, so a vacated cell whose valid neighbors were all distinct features, or which had a single valid neighbor, was never filled and the dilation loop never terminated (GitHub issue #1698). | DREAM3D-NX v7.0.0 through v7.4.1. DREAM.3D 6.5.171 was not affected. | Fixed in v7.4.2 by PR #1700 (first sighting counts as a hit). This branch adds the `Fill from distinct first-sighting neighbors (issue #1698)` regression test that reproduces the issue's 5x2x1 case. | +| `RemoveFlaggedFeaturesFilter-D2` | With fill on, any cell with FeatureId 0 was treated as an unresolved cell but never filled, so the dilation loop never terminated on any input that contained background cells. | DREAM3D-NX v7.0.0 through v7.4.2. DREAM.3D 6.5.171 was not affected. | The vacated-cell test is `featureName >= 0 -> skip`, matching 6.5.171. Pinned by `Fill treats FeatureId 0 as a source and never a target`, which hung before the fix. | +| `RemoveFlaggedFeaturesFilter-D3` | With fill on, when every cell belonged to a flagged feature and the only unflagged features owned no cells, no vacated cell had a fill source and the loop spun forever. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | A pass that leaves vacated cells but records no fill source returns error `-45436`. Pinned by `Execute Error - no fill progress (-45436)`. | +| `RemoveFlaggedFeaturesFilter-D4` | A FeatureId that was negative or not less than the feature tuple count indexed the flag vector out of bounds during marking. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | Every FeatureId is validated before any modification; a bad value returns error `-45435` naming the cell, value and valid range. Pinned by `Execute Error - FeatureId out of range (-45435)`. | +| `RemoveFlaggedFeaturesFilter-D5` | The extract path tested the preflight result where it should have tested the execute result of its sub-filters, so an execute failure was ignored, and a preflight failure threw `std::runtime_error` instead of returning an error. | DREAM3D-NX v7.0.0 through v7.4.2. Not applicable to 6.5.171. | Sub-filter preflight and execute results are checked and returned as errors `-53901` through `-53904` with the sub-filter's message. Covered by inspection; see Code path coverage rows 6, 7, 11, 12. | +| `RemoveFlaggedFeaturesFilter-D6` | A flagged feature that owns no cell has an empty bounding box. SIMPLNX passed it to the crop, which failed preflight and threw. DREAM.3D 6.5.171 crops a 1x1x1 geometry at origin (-1, -1, -1) containing cell 0. | DREAM.3D 6.5.171 (bogus geometry); DREAM3D-NX v7.0.0 through v7.4.2 (uncaught exception). | The feature is skipped with warning `-53905` and no geometry is created. Pinned by `Extract - flagged feature with no cells is skipped with a warning`. | + +A preflight cleanup that is not a deviation: the filter emitted its own NeighborList warning (`-11505`) and the shared helper emitted a second one (`-5558`) for the same arrays. The duplicate was removed; the test asserts exactly one warning. + +## Code path coverage + +20 of 28 paths are assertion-covered. The uncovered paths are cancellation and defensive failure paths of the sub-filters and the compaction helper. + +Source: `src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp` (222 lines) and `src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp` (333 lines), plus preflight in `Filters/RemoveFlaggedFeaturesFilter.cpp` (212 lines). + +Phases: (a) preflight, (b) extract (bounding boxes, then one crop per flagged feature), (c) removal marking, (d) iterative fill, (e) compaction. + +| # | Phase | Path | Test case | +|---|---|---|---| +| 1 | (a) Preflight | Parent of the flag array is not an Attribute Matrix -> error `-9892` | `Preflight Error - flag array parent is not an Attribute Matrix (-9892)` | +| 2 | (a) Preflight | *Extract* or *Extract then Remove* -> temporary `tempBounds` array created and deleted after execution | `Class 1 Oracle - Extract crops the bounding box of each flagged feature` (asserts `tempBounds` is gone) | +| 3 | (a) Preflight | *Remove* or *Extract then Remove* -> cell and feature groups marked modified, NeighborLists scheduled for deletion, one warning `-5558` | `Preflight Warning - NeighborLists are removed (-5558)` (`Functionality = 0`, `Functionality = 2`) | +| 4 | (a) Preflight | *Extract* -> no NeighborList warning, NeighborLists kept | `Preflight Warning - NeighborLists are removed (-5558)` (`Extract alone does not remove NeighborLists`) | +| 5 | (b) Extract | Flag array is not Bool or UInt8 -> error `-53900` | *Not directly tested. The parameter type check in preflight prevents it; defensive.* | +| 6 | (b) Extract | `ComputeFeatureRect` preflight fails -> error `-53901` | *Not directly tested. Requires an internal inconsistency the filter's own preflight already rejects; defensive.* | +| 7 | (b) Extract | `ComputeFeatureRect` execute fails -> error `-53902` | *Not directly tested. The sub-filter fails only on a FeatureId beyond the feature tuple count, which path 14 rejects first in the removal modes; defensive in Extract-only mode.* | +| 8 | (b) Extract | Flagged feature owns no cell -> warning `-53905`, no geometry | `Extract - flagged feature with no cells is skipped with a warning` | +| 9 | (b) Extract | Flagged feature -> new geometry `-` with the bounding-box dimensions, origin, all cell arrays and the feature Attribute Matrix | `Class 1 Oracle - Extract crops the bounding box of each flagged feature`, `Class 1 Oracle - Extract zero-pads names and handles several features`, `Class 1 Oracle - Extract then Remove with fill` | +| 10 | (b) Extract | Unflagged feature -> no geometry | `Class 1 Oracle - Extract zero-pads names and handles several features` | +| 11 | (b) Extract | Crop preflight fails -> error `-53903` | *Not directly tested. Reachable before this branch through path 8; now unreachable for valid bounds; defensive.* | +| 12 | (b) Extract | Crop execute fails -> error `-53904` | *Not directly tested. Defensive.* | +| 13 | (b) Extract | Cancel between crops -> return | *Not directly tested. Requires cancel-signal injection.* | +| 14 | (c) Marking | A FeatureId is negative or >= feature tuple count -> error `-45435`, nothing modified | `Execute Error - FeatureId out of range (-45435)` (both sections) | +| 15 | (c) Marking | Every feature flagged -> error `-45433`, nothing modified | `Execute Error - all features flagged (-45433)` (`Fill = false`, `Fill = true`) | +| 16 | (c) Marking | Fill off: flagged cell -> 0 | `Class 1 Oracle - Remove without fill` | +| 17 | (c) Marking | Fill on: flagged cell -> -1 | Every `Class 1 Oracle - Fill *` test | +| 18 | (d) Fill | Vacated cell with a single valid neighbor, or all-distinct neighbors: first sighting records the source | `Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` (cells 3 and 9) | +| 19 | (d) Fill | Majority vote: the source moves only when a feature's tally strictly exceeds the previous maximum | `Class 1 Oracle - Fill majority vote and second iteration (3x3x3)` (cell 13, 4 to 2) | +| 20 | (d) Fill | 1-vs-1 tie -> first-seen feature in -Z, -Y, -X, +X, +Y, +Z order | `Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` (cell 4), `(3x3x3)` (cell 0) | +| 21 | (d) Fill | Vacated cell with no non-negative neighbor this pass -> filled in a later pass from a neighbor that was itself filled | `(issue #1698)` (cell 4 via cell 3), `(3x3x3)` (cell 0 via cell 9 via cell 18) | +| 22 | (d) Fill | Background cell (FeatureId 0) is skipped as a target and counted as a source | `Class 1 Oracle - Fill treats FeatureId 0 as a source and never a target` (cell 14 -> 0) | +| 23 | (d) Fill | Vacated cells remain and none has a source -> error `-45436`, cells left at -1 | `Execute Error - no fill progress (-45436)` | +| 24 | (d) Fill | Every non-ignored cell array is copied from the source cell; ignored arrays are untouched | `Class 1 Oracle - Fill copies every non-ignored Cell array` and the `IgnoredValue` checks in every fill test | +| 25 | (d) Fill | Face neighbors outside the volume are excluded | 5x2x1 and 4x4x1 fixtures (Y and Z faces), 3x3x3 fixture (all six faces) | +| 26 | (d) Fill | Cancel during a pass -> return | *Not directly tested. Requires cancel-signal injection.* | +| 27 | (e) Compaction | Surviving features renumbered contiguously from 1 in input order; feature arrays compacted in place; NeighborLists deleted | Every remove test (`CheckFeatureArraysCompacted`), `Class 4 Invariants - Small IN100 remove small features with fill` | +| 28 | (e) Compaction | `RemoveInactiveObjects` fails -> error `-45434` | *Not directly tested. Requires a feature group whose arrays do not match the flag count, which the preflight tuple check rejects; defensive.* | + +## Test inventory + +| Test case | Status | Notes | +|-----------|--------|-------| +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Remove without fill` | new-for-V&V | Two sections (fixtures A and B). Asserts exact `FeatureIds`, unchanged `CellValue` and `IgnoredValue`, and the compacted `Int32DataSet` and flag arrays. 36 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` | new-for-V&V | The issue's 5x2x1 case. Asserts exact `FeatureIds`, the copy source of each of the three filled cells via `CellValue`, the untouched ignored array and the compaction. Hung before v7.4.2. 19 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill copies every non-ignored Cell array` | new-for-V&V | Fixture A with an empty ignore list; asserts `IgnoredValue` follows the same copies as `CellValue`. 15 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill treats FeatureId 0 as a source and never a target` | new-for-V&V | Fixture B with fill; asserts the five background cells stay 0, cell 13 copies from cell 12 and cell 14 copies from background cell 15. Hung before this branch (D2). 19 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill majority vote and second iteration (3x3x3)` | new-for-V&V | Fixture E; asserts all 27 `FeatureIds`, the five copy sources including the two-pass chain into cell 0, ignored array and compaction. 19 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - all features flagged (-45433)` | new-for-V&V | `DYNAMIC_SECTION` for fill off and on; asserts the exact code and that no array was modified. 21 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - FeatureId out of range (-45435)` | new-for-V&V | Sections for a value equal to the tuple count and for a negative value; asserts the code and that `FeatureIds` is unmodified. Covers D4. 15 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - no fill progress (-45436)` | new-for-V&V | 4x1x1 where the only unflagged feature owns no cell; asserts the code and that all cells are left at -1. Covers D3. 8 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract crops the bounding box of each flagged feature` | new-for-V&V | Fixture B extract; asserts the original is untouched, the new geometry's dimensions (2,1,1), origin (1,3,0), spacing, all three cell arrays, the carried feature Attribute Matrix, and deletion of `tempBounds`. 35 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract zero-pads names and handles several features` | new-for-V&V | 12-feature 1D fixture; asserts two-digit padding (`-03`, `-11`), per-feature dimensions, origin and cell values, and that unflagged features produce no geometry. 39 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Extract - flagged feature with no cells is skipped with a warning` | new-for-V&V | Asserts warning `-53905`, that feature 3 is still extracted and feature 4 produces no geometry. Covers D6. 24 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract then Remove with fill` | new-for-V&V | Fixture B; asserts the extracted geometry (from the unmodified input) and the filled original in one run. 31 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Error - flag array parent is not an Attribute Matrix (-9892)` | new-for-V&V | Flag array placed in a plain `DataGroup`; asserts invalid preflight and the code. 5 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Warning - NeighborLists are removed (-5558)` | new-for-V&V | `DYNAMIC_SECTION` for *Remove* and *Extract then Remove* asserts exactly one warning with code `-5558` and that the NeighborList is gone after execution; a third section asserts *Extract* emits no warning and keeps it. 23 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 4 Invariants - Small IN100 remove small features with fill` | new-for-V&V | Loads `6_5_test_data_1_v2`, flags the 275 features with fewer than 100 cells, fills, and checks the six invariants listed in the Oracle section over 1,000,000 cells with aggregated counts. 751 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: SIMPL Backwards Compatibility` | kept | Two `DYNAMIC_SECTION`s (`SIMPL 6.5 (UUID)`, `SIMPL 6.4 (Filter_Name)`); modified this cycle to also assert the two converted ignored-array paths. 23 assertions. | +| `SimplnxCore::RemoveFlaggedFeatures: Test Remove Algorithm` | retired | Ran *Remove* with fill off on the 4x4x1 fixture and checked `FeatureIds` and the compacted feature array. Replaced by `Class 1 Oracle - Remove without fill` (fixture B), which adds the cell-array and ignored-array checks. The fill path was never exercised. | +| `SimplnxCore::RemoveFlaggedFeatures: Test Extract Algorithm` | retired | Checked only two `FeatureIds` values in the extracted geometry. Replaced by `Class 1 Oracle - Extract crops the bounding box of each flagged feature`, which also checks dimensions, origin, spacing, every cell array and the feature Attribute Matrix. | +| `SimplnxCore::RemoveFlaggedFeatures: Test Extract then Remove Algorithm` | retired | Fill off; same two-value extract check. Replaced by `Class 1 Oracle - Extract then Remove with fill`. | + +There are 16 active `TEST_CASE`s: 1 kept (modified) and 15 new for this V&V cycle; 3 retired. + +## Exemplar archive + +No output archive was created. Both Class 1 fixtures and their expected values are encoded inline. + +The Class 4 test uses this shared input archive: + +| Field | Value | +|---|---| +| Archive | `6_5_test_data_1_v2.tar.gz` | +| SHA512 | `585b51ba1da9784a204fe88073ca562b45afd7007cf451b0193079b885c4b4caff7cf21b13e016433b84155546ac0f73f003a8b8ebb1c58360b2c56de3027d6c` | +| CMake registration | `src/Plugins/SimplnxCore/test/CMakeLists.txt` | +| Provenance | `src/Plugins/SimplnxCore/vv/provenance/6_5_test_data_1_v2.md` | +| Role in this V&V | Input only. The test derives the flag array, the expected compaction and every invariant from the pre-filter arrays. No archived array is compared against filter output. | + +## Deviations from DREAM3D 6.5.171 + +On 2026-09-02, DREAM3D 6.5.171 and SIMPLNX were run on legacy-native copies of fixtures A, B and E with fill on and off (five removal cases) and on fixture B with *Extract*. The inputs were authored with the shared h5py legacy writer so both runners read identical files. All 25 removal array comparisons (`FeatureIds`, `CellValue`, `IgnoredValue`, `Int32DataSet`, `Active`) matched the analytical oracle and each other exactly. The extracted geometry's dimensions, origin and three cell arrays matched. Three further fixtures were run to characterize the error-path deviations: the no-progress twin (D3), an out-of-range FeatureId (D4) and a flagged feature with no cells (D6). The pipelines, inputs, outputs, logs and comparison script are archived outside the repository (`FilterVerification_RemoveFlaggedFeaturesFilter`) per the archival policy. + +See `vv/deviations/RemoveFlaggedFeaturesFilter.md` for the root cause, affected users and recommendation for each deviation. + +| Deviation | Observed difference | +|-----------|---------------------| +| `RemoveFlaggedFeaturesFilter-D1` | SIMPLNX v7.0.0 through v7.4.1 never returned when a vacated cell's valid neighbors were all distinct features or a single neighbor; 6.5.171 completed. Fixed in v7.4.2. | +| `RemoveFlaggedFeaturesFilter-D2` | SIMPLNX through v7.4.2 never returned when the input contained FeatureId 0 cells and fill was on; 6.5.171 completed. Fixed on this branch. | +| `RemoveFlaggedFeaturesFilter-D3` | SIMPLNX returns `-45436` when no vacated cell has a fill source; 6.5.171 loops forever. | +| `RemoveFlaggedFeaturesFilter-D4` | SIMPLNX returns `-45435` for a FeatureId outside `[0, tuple count)`; 6.5.171 reads out of bounds and silently zeroed the cell in the A/B run. | +| `RemoveFlaggedFeaturesFilter-D5` | SIMPLNX returns `-53901` through `-53904` when an extract sub-filter fails; earlier SIMPLNX ignored execute failures and threw on preflight failures. Not applicable to 6.5.171. | +| `RemoveFlaggedFeaturesFilter-D6` | For a flagged feature with no cells SIMPLNX warns `-53905` and creates nothing; 6.5.171 creates a 1x1x1 geometry at origin (-1, -1, -1) holding cell 0. | +| `RemoveFlaggedFeaturesFilter-D7` | Extracted geometries are named `-` and carry the feature Attribute Matrix; 6.5.171 names them `Feature_` and copies only cell data. | + +Behavior that is identical in both implementations and is recorded here because it surprises users: background cells (FeatureId 0) are legal fill sources. A vacated cell whose non-negative neighbors are all background becomes background. This is the documented isotropic-coarsening rule shared with `RequireMinimumSizeFeatures` and `RequireMinNumNeighbors`. diff --git a/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md new file mode 100644 index 0000000000..62154d3f56 --- /dev/null +++ b/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md @@ -0,0 +1,133 @@ +# Deviations from DREAM3D 6.5.171: RemoveFlaggedFeaturesFilter + +This file lists every documented behavioral difference between SIMPLNX `RemoveFlaggedFeaturesFilter` and its DREAM3D 6.5.171 equivalents, `RemoveFlaggedFeatures` (*Remove*) and `ExtractFlaggedFeatures` (*Extract*). + +Entries are referenced by stable ID (`RemoveFlaggedFeaturesFilter-D`) from the V&V report and from public migration guidance. The ID is stable across renames; the Filter UUID field is the permanent cross-reference anchor. + +On valid input (every FeatureId in range, at least one unflagged feature that owns cells, every flagged feature owning at least one cell) the removal and fill outputs of the two implementations are bit-identical on every fixture run, and the extracted geometry's dimensions, origin and cell arrays match. The deviations below concern inputs outside that envelope, two SIMPLNX-only defects that are fixed, and the packaging of extracted geometries. + +--- + +## RemoveFlaggedFeaturesFilter-D1 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D1` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | retired 2026-08-13 (fixed in DREAM3D-NX v7.4.2 by PR #1700); regression test added 2026-09-02 | + +**Symptom:** With *Fill-in Removed Features* enabled, DREAM3D-NX v7.0.0 through v7.4.1 never returned when a vacated cell's valid face neighbors all belonged to distinct features, or when it had a single valid neighbor. DREAM3D 6.5.171 completed and filled the cell. Reported as GitHub issue #1698. + +**Root cause:** `bug` in SIMPLNX. The per-cell tally in `IdentifyNeighbors()` pushed a newly seen feature onto `discoveredFeatures` without incrementing its hit count, so a fill source was only recorded on the second sighting of the same feature. A cell with no repeated neighbor feature never received a source, `FindVoxelArrays()` copied nothing into it, it stayed at -1, and the `do { } while(shouldLoop)` loop repeated the same pass forever. The legacy `n[feature]++` tally counted the first sighting. On large 3D volumes most vacated cells have two face neighbors in the same feature, which hid the defect; it surfaces on thin volumes and on small or isolated removed regions. + +**Affected users:** Anyone who enabled *Fill-in Removed Features* on DREAM3D-NX v7.0.0 through v7.4.1 with thin geometries (one slice or one voxel deep) or with small isolated removed features. The pipeline had to be killed; no output was written. + +**Recommendation:** `trust SIMPLNX` v7.4.2 or later, whose output is identical to 6.5.171 on the issue's 5x2x1 case and on every other fixture in this V&V. Users on v7.4.1 or earlier must upgrade or disable fill. + +--- + +## RemoveFlaggedFeaturesFilter-D2 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D2` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | retired 2026-09-02 (fixed on the V&V branch) | + +**Symptom:** With *Fill-in Removed Features* enabled, DREAM3D-NX v7.0.0 through v7.4.2 never returned when the Feature Ids array contained any cell with FeatureId 0. DREAM3D 6.5.171 completed and left the background cells at 0. + +**Root cause:** `bug` in SIMPLNX. `IdentifyNeighbors()` skipped cells with `featureName > 0`, so background cells (0) were treated as unresolved and set `shouldLoop`, but `FindVoxelArrays()` only copies into cells with `featureName < 0`. Background cells were therefore never overwritten and every pass ended with `shouldLoop == true`. The legacy loop tests `featurename < 0`. SIMPLNX now tests `featureName >= 0 -> skip`, matching 6.5.171: background is never a fill target but remains a legal fill source (see the note at the end of the V&V report). + +**Affected users:** Anyone who enabled fill on data with unassigned (background) cells, which is the common case after a segmentation with a mask. The pipeline had to be killed. + +**Recommendation:** `trust SIMPLNX` at or after this fix; its output is identical to 6.5.171 on the 4x4x1 background fixture. Users on v7.4.2 or earlier must disable fill on data with background cells. + +--- + +## RemoveFlaggedFeaturesFilter-D3 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D3` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | active | + +**Symptom:** With fill enabled, when every cell belongs to a flagged feature and the unflagged features own no cells, SIMPLNX returns error `-45436` and leaves the cells at -1; DREAM3D 6.5.171 loops forever. Reproduced on a 4x1x1 volume with `FeatureIds = [1, 1, 1, 1]`, three feature tuples, and only feature 1 flagged: 6.5.171 was still running when killed after 30 seconds. + +**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). The all-flagged guard only inspects the flag array, so an unflagged feature that owns no cells satisfies it. After marking, no cell is non-negative, so no vacated cell ever finds a fill source and neither implementation detected the lack of progress. SIMPLNX now tracks whether a pass recorded any fill source and stops with a deterministic error when vacated cells remain without one. This is the same class of defect as `RequireMinNumNeighborsFilter-D3`. + +**Affected users:** Users whose feature Attribute Matrix contains features that own no cells, when the flag array removes every feature that does own cells. Valid inputs are unaffected. + +**Recommendation:** `trust SIMPLNX`. The error names the array and explains the two ways to correct the input; the legacy behavior is an unbounded execution. + +--- + +## RemoveFlaggedFeaturesFilter-D4 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D4` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | active | + +**Symptom:** When a FeatureId is negative or not less than the feature tuple count, SIMPLNX returns error `-45435` before modifying anything; DREAM3D 6.5.171 indexes its flag vector out of bounds. In the A/B run (5x2x1, cell 6 set to 5 with five feature tuples) 6.5.171 completed and silently zeroed that cell, which depends on whatever memory followed the vector. + +**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). `remove_flaggedfeatures()` evaluates `activeObjects[gnum]` for every cell without validating `gnum`. SIMPLNX now validates every FeatureId against the flag count in a read-only pass before the marking pass. This is the same class of defect as `RequireMinNumNeighborsFilter-D2`. + +**Affected users:** Users with a Feature Ids array that does not correspond to the selected feature Attribute Matrix, for example after selecting the wrong Attribute Matrix or after a partial renumbering. Valid inputs are unaffected. + +**Recommendation:** `trust SIMPLNX`. The error names the cell, the value and the valid range, and no data is modified. + +--- + +## RemoveFlaggedFeaturesFilter-D5 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D5` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | retired 2026-09-02 (fixed on the V&V branch) | + +**Symptom:** In *Extract* and *Extract then Remove*, DREAM3D-NX v7.0.0 through v7.4.2 ignored a failure of the bounding-box or crop sub-filter's execute step and continued with unset bounds, and reported a sub-filter preflight failure by throwing `std::runtime_error` rather than returning a filter error. Not applicable to 6.5.171, whose extract filter calls `CropImageGeometry` directly and checks no result. + +**Root cause:** `bug` in SIMPLNX. After each `filter.execute()` the code tested `preflightResult.outputActions.invalid()` instead of `executeResult.result.invalid()`. SIMPLNX now returns errors `-53901`/`-53902` (bounding box preflight/execute) and `-53903`/`-53904` (crop preflight/execute) carrying the sub-filter's own message. + +**Affected users:** Users whose extract run hit a sub-filter failure, chiefly the empty-feature case in D6. Successful extractions were unaffected. + +**Recommendation:** `trust SIMPLNX`. Failures are now reported as ordinary filter errors with the cause attached. + +--- + +## RemoveFlaggedFeaturesFilter-D6 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D6` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | active | + +**Symptom:** When a flagged feature owns no cell, SIMPLNX emits warning `-53905` and creates no geometry for it; DREAM3D 6.5.171 creates a 1x1x1 geometry named `Feature_` at origin (-1, -1, -1) that contains cell 0 of the source volume. Reproduced on the 4x4x1 fixture with an unused fifth feature flagged: 6.5.171 produced `Feature_4` with `FeatureIds = [0]` and `CellValue = [100]`. DREAM3D-NX v7.0.0 through v7.4.2 threw an uncaught exception on the same input (see D5). + +**Root cause:** `bug` in DREAM3D 6.5.171 (bogus output) and in SIMPLNX before this branch (uncaught exception). Legacy `find_feature_bounds()` initializes every bound to -1 and only updates bounds for features that own cells, so an empty feature keeps (-1, -1, -1) to (-1, -1, -1), which `CropImageGeometry` accepts as a one-voxel crop. In SIMPLNX `ComputeFeatureRect` leaves minimum > maximum for an empty feature; the crop preflight rejected that and the algorithm threw. SIMPLNX now detects minimum > maximum, warns, and skips the feature. + +**Affected users:** Users extracting from a feature Attribute Matrix with unused tuples, for example after a filter that removed features without compacting, or with a flag array that marks unused ids. The legacy output contains a geometry that does not correspond to any feature. + +**Recommendation:** `trust SIMPLNX`. The warning names the feature and the array; no spurious geometry is written. + +--- + +## RemoveFlaggedFeaturesFilter-D7 + +| Field | Value | +|---|---| +| **Deviation ID** | `RemoveFlaggedFeaturesFilter-D7` | +| **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | +| **Status** | active | + +**Symptom:** An extracted geometry is named `-` with the id zero-padded to the width of the feature count (for example `Extracted_Feature-03` when there are 12 feature tuples) and carries a copy of the source feature Attribute Matrix; DREAM3D 6.5.171 names it `Feature_` with no padding and copies only the cell Attribute Matrix. The geometry's dimensions, origin, spacing and cell arrays are identical in both. + +**Root cause:** `algorithmic choice`. SIMPLNX exposes the prefix as a parameter and pads so that geometries sort in id order; it reuses `CropImageGeometryFilter` with feature renumbering off, which copies the feature Attribute Matrix so feature-level attributes stay available in the extracted geometry. + +**Affected users:** Pipelines or scripts that reference extracted geometries by the legacy `Feature_` name, and users who expect the extracted geometry to contain only cell data. + +**Recommendation:** `trust SIMPLNX`. Set the prefix to `Feature_` for a closer match; note that the id is still zero-padded and the feature Attribute Matrix is still carried. diff --git a/src/Plugins/SimplnxCore/vv/provenance/6_5_test_data_1_v2.md b/src/Plugins/SimplnxCore/vv/provenance/6_5_test_data_1_v2.md index ee87c4a1e0..8d1105b394 100644 --- a/src/Plugins/SimplnxCore/vv/provenance/6_5_test_data_1_v2.md +++ b/src/Plugins/SimplnxCore/vv/provenance/6_5_test_data_1_v2.md @@ -9,7 +9,7 @@ This sidecar records the known provenance and test roles of the shared `6_5_test | **Archive** | `6_5_test_data_1_v2.tar.gz` | | **SHA512** | `585b51ba1da9784a204fe88073ca562b45afd7007cf451b0193079b885c4b4caff7cf21b13e016433b84155546ac0f73f003a8b8ebb1c58360b2c56de3027d6c` | | **Archive member** | `6_5_test_data_1_v2/6_5_test_data_1_v2.dream3d` | -| **Used by tests** | 19 active test cases across five SimplnxCore test source files | +| **Used by tests** | 20 active test cases across six SimplnxCore test source files | | **Generated by** | Unknown. The archive was created before archive-level provenance was recorded. | | **Generated on** | Unknown. The first repository reference was committed on 2024-07-18. | | **First repository reference** | PR [#1021](https://github.com/BlueQuartzSoftware/simplnx/pull/1021), commit `c3be139e2ca0bbbb151d239834e8364ad70aaad1` | @@ -56,6 +56,7 @@ For the `RequireMinNumNeighborsFilter` regression, the relevant input arrays are | `test/CreateFeatureArrayFromElementArrayTest.cpp` | 2 | Supplies the Small IN100 input and archived feature-level `ConfidenceIndex` and `IPFColors` regression values | | `test/CropImageGeometryTest.cpp` | 14 | Supplies `DataContainer2` and the seven cropped geometries used as regression references for voxel-index and physical-bounds variants | | `test/RequireMinNumNeighborsTest.cpp` | 1 | Supplies input data only; expected retained `NumElements` values are derived in the test before the filter runs | +| `test/RemoveFlaggedFeaturesTest.cpp` | 1 | Supplies input data only for the Class 4 invariant test; the flag array (features with fewer than 100 cells), the expected compaction and every invariant are derived from the pre-filter arrays | | `test/ScalarSegmentFeaturesTest.cpp` | 1 | Supplies the Small IN100 scalar input; the test checks that 847 feature tuples are produced | ## Canonical Oracle Output From abd495a4acb54efb0a17ac7a617e21d204789e1b Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Wed, 2 Sep 2026 16:02:21 -0400 Subject: [PATCH 2/3] REV: Apply review findings to the Remove/Extract Flagged Features V&V Five scoped review passes (adversarial, senior engineer, CPU, memory, out-of-core) were run on the V&V change set. Findings applied: * Fill loop: measure progress by the number of cells actually filled, not by whether a source was chosen. Listing the Feature Ids array in the ignored arrays still hung the loop; that array is now always copied and the caller is warned (-45438). * Validate that the Feature Ids tuple count equals the geometry cell count (-45437) and reject a feature Attribute Matrix with fewer than two tuples (-45433) before any modification; poll the cancel flag in the validation pass; report the stuck-cell count in the -45436 message. * Replace the per-vacated-cell std::vector tallies with fixed std::array storage, build the cell-array list once per run, and make the neighbor scan's inputs const. * Extract: copy the bounds into a local buffer and delete the temporary bounds array before the first crop, so it is neither copied into every extracted geometry nor left behind on an error return; aggregate the empty-feature warning (-53905) into one message; carry the crop bounds and a further-error count in sub-filter messages; take the bounds store by const reference. * Preflight: remove the dead CreateArrayAction (sized to the cell count) and the deferred delete that the algorithm now performs itself. * Tests: add the Feature-Ids-ignored, tuple-count-mismatch and aggregated warning cases, assert the extracted geometry carries no tempBounds, and add a background-cell fill section to KeepRemoveRankedFeaturesTest, which shares the fill utility. * Docs and V&V report: correct the deviation counts, the tempBounds and ordering claims, add the out-of-core build caveat, drop stale line counts, and document the new codes in both filters' user documentation. Signed-off-by: Michael Jackson --- .../docs/KeepRemoveRankedFeaturesFilter.md | 8 + .../docs/RemoveFlaggedFeaturesFilter.md | 14 +- .../Algorithms/RemoveFlaggedFeatures.cpp | 95 ++++-- .../Filters/RemoveFlaggedFeaturesFilter.cpp | 17 +- .../utils/FeatureRemovalUtilities.cpp | 271 +++++++++++------- .../utils/FeatureRemovalUtilities.hpp | 10 +- .../test/KeepRemoveRankedFeaturesTest.cpp | 24 ++ .../test/RemoveFlaggedFeaturesTest.cpp | 84 +++++- .../vv/RemoveFlaggedFeaturesFilter.md | 65 +++-- .../deviations/RemoveFlaggedFeaturesFilter.md | 8 +- 10 files changed, 406 insertions(+), 190 deletions(-) diff --git a/src/Plugins/SimplnxCore/docs/KeepRemoveRankedFeaturesFilter.md b/src/Plugins/SimplnxCore/docs/KeepRemoveRankedFeaturesFilter.md index d94cbed4c8..51174855d0 100644 --- a/src/Plugins/SimplnxCore/docs/KeepRemoveRankedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/docs/KeepRemoveRankedFeaturesFilter.md @@ -74,6 +74,14 @@ among the tied **Features** is arbitrary. Ties are common in integer arrays such - The filter warns during preflight when the chosen settings would remove nothing, and when they would remove every **Feature**. The latter is an error at execute time, since it would leave nothing behind. +- Every value in *Cell Feature Ids* must be in the range 0 through (number of **Feature** tuples - 1), + and the array must hold one value per **Cell** of the selected geometry. A value outside that range + stops the filter with error *-45435*, and a tuple-count mismatch with error *-45437*, before any data + is modified. These checks are shared with [Remove/Extract Flagged Features](RemoveFlaggedFeaturesFilter.md), + which also documents the fill rules (background **Cells** are never filled but can be fill sources) and + the *-45436* error that stops a fill that cannot make progress. +- The *Cell Feature Ids* array is always copied by the fill, even when it is listed in *Attribute Arrays + to Ignore*; the filter warns (*-45438*) and removes it from the list. % Auto generated parameter table will be inserted here diff --git a/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md index 528dcac9f1..f83deb0a27 100644 --- a/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/docs/RemoveFlaggedFeaturesFilter.md @@ -6,7 +6,7 @@ Processing (Cleanup) ## Description -This **Filter** removes or extracts **Features** that an earlier filter in the pipeline has flagged. The user supplies a boolean (or unsigned 8-bit) array at the **Feature** level; features with a value of *true* are targeted for removal or extraction. This filter is the general-purpose tool for discarding features based on any criterion that can be expressed as a flag: biased features, surface features, minority phases, user-selected grains, and so on. For the specific case of removing features smaller than a size threshold, see [Remove Minimum Size Features](RequireMinimumSizeFeaturesFilter.md). For a rank-based selection ("remove the 10 smallest"), see [Keep/Remove Ranked Features](KeepRemoveRankedFeaturesFilter.md). +This **Filter** removes or extracts **Features** that an earlier filter in the pipeline has flagged. The user supplies a boolean (or unsigned 8-bit) array at the **Feature** level. Features with a value of *true* are removed or extracted. This filter discards features by any criterion that a flag can express: biased features, surface features, minority phases, or user-selected features. For the specific case of removing features smaller than a size threshold, see [Remove Minimum Size Features](RequireMinimumSizeFeaturesFilter.md). For a rank-based selection ("remove the 10 smallest"), see [Keep/Remove Ranked Features](KeepRemoveRankedFeaturesFilter.md). ### Selected Operation @@ -18,17 +18,17 @@ The *Selected Operation* parameter provides the following choices: ### How Fill-in Removed Features Works -When *Fill-in Removed Features* is on, the vacated **Cells** are filled by isotropic coarsening: the neighboring features grow into the gap one layer per pass until no vacated cell remains. +When *Fill-in Removed Features* is on, isotropic coarsening fills the vacated **Cells**. The neighboring features grow into the gap one layer per pass until no vacated cell remains. 1. Every **Cell** of a flagged **Feature** is marked as vacated. 2. Each vacated cell polls its six face neighbors in the order -Z, -Y, -X, +X, +Y, +Z. Neighbors outside the volume and neighbors that are themselves vacated are ignored. -3. The cell copies every **Cell** level array (except those listed in *Attribute Arrays to Ignore*) from the neighbor whose feature is the most common among the polled neighbors. A tie goes to the feature seen first in the order above. +3. The cell copies every **Cell** level array (except those listed in *Attribute Arrays to Ignore*) from the neighbor whose feature is the most common among the polled neighbors. A tie goes to the feature seen first in the order above. The *Cell Feature Ids* array is always copied, even if it is listed; the filter warns (*-45438*) and removes it from the list. 4. A vacated cell with no usable neighbor waits for the next pass, when its own neighbors have been filled. 5. Passes repeat until no vacated cell remains. Background cells (FeatureId 0) are never vacated and are never filled, but they do count as neighbors: a vacated cell whose only usable neighbors are background becomes background. This matches DREAM3D 6.5.171 and the other coarsening filters. -If a pass cannot fill any remaining vacated cell, which happens only when every cell belonged to a flagged feature and the unflagged features own no cells, the filter stops with error *-45436* instead of running forever. +A pass can fail to fill any remaining vacated cell. This happens only when every cell belonged to a flagged feature and the unflagged features own no cells. The filter then stops with error *-45436* instead of running forever. ### Extract Details @@ -38,7 +38,7 @@ A flagged **Feature** that owns no **Cells** has nothing to extract. The filter ### Input Validation -Every value in *Cell Feature Ids* must be in the range 0 through (number of feature tuples - 1). A value outside that range stops the filter with error *-45435* before any data is modified. Flagging every **Feature** stops the filter with error *-45433*. +Every value in *Cell Feature Ids* must be in the range 0 through (number of feature tuples - 1), and the array must hold one value per **Cell** of the selected geometry. A value outside that range stops the filter with error *-45435*, and a tuple-count mismatch stops it with error *-45437*, before any data is modified. Flagging every **Feature** stops the filter with error *-45433*. ### WARNING: NeighborList Removal @@ -51,12 +51,14 @@ This filter will **only** run on an **Image Geometry**. ### Required Input Sources - **Cell Feature Ids** -- produced by a segmentation filter such as [Segment Features (Misorientation)](../OrientationAnalysis/EBSDSegmentFeaturesFilter.md) or [Segment Features (Scalar)](ScalarSegmentFeaturesFilter.md). -- **Flagged Features** -- a boolean feature-level array. Typical producers: [Compute Biased Features](ComputeBiasedFeaturesFilter.md), [Compute Surface Features](ComputeSurfaceFeaturesFilter.md), or a custom flag built with threshold or boolean operations on any feature-level statistic. +- **Flagged Features** -- a boolean feature-level array. Typical producers: [Compute Biased Features (Bounding Box)](ComputeBiasedFeaturesFilter.md), [Compute Surface Features](ComputeSurfaceFeaturesFilter.md), or a custom flag built with threshold or boolean operations on any feature-level statistic. % Auto generated parameter table will be inserted here ## Example Pipelines +None. + ## Differences from DREAM3D 6.5.171 For valid input, the removal and fill outputs match DREAM3D 6.5.171 exactly, and extracted geometries have the same dimensions, origin and cell data. SIMPLNX differs in how it handles malformed input and in how it packages extracted geometries: diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp index 3847c28b5f..5ffab33492 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp @@ -18,13 +18,38 @@ constexpr int32 k_CropPreflightError = -53903; constexpr int32 k_CropExecuteError = -53904; constexpr int32 k_EmptyFeatureSkippedWarning = -53905; +/// Number of empty feature ids listed in the -53905 warning before the list is truncated. +constexpr usize k_MaxListedEmptyFeatures = 10; + +/** + * @brief Formats the first error of a sub-filter result for embedding in this filter's message. + * @param errors Errors returned by the sub-filter. + * @return "[code] message", with a count of any further errors appended. + */ std::string FirstErrorMessage(const std::vector& errors) { - return errors.empty() ? std::string("(no error message)") : fmt::format("[{}] {}", errors[0].code, errors[0].message); + if(errors.empty()) + { + return "(no error message)"; + } + std::string message = fmt::format("[{}] {}", errors[0].code, errors[0].message); + if(errors.size() > 1) + { + message += fmt::format(" (+{} more)", errors.size() - 1); + } + return message; } /** * @brief Crops one feature's bounding box out of the source Image Geometry into a new geometry. + * + * @param dataStructure Receives the new geometry. + * @param shouldCancel Checked between the crop preflight and execute. + * @param imageGeometryPath The source Image Geometry. + * @param minVoxels Inclusive minimum (x, y, z) voxel indices of the bounding box. + * @param maxVoxels Inclusive maximum (x, y, z) voxel indices of the bounding box. + * @param createdImgGeomPath Path of the new geometry. + * @return Error -53903 if the crop preflight fails, -53904 if its execute fails, otherwise valid. */ Result<> CropFeature(DataStructure& dataStructure, const std::atomic_bool& shouldCancel, const DataPath& imageGeometryPath, const std::vector& minVoxels, const std::vector& maxVoxels, const DataPath& createdImgGeomPath) @@ -42,11 +67,12 @@ Result<> CropFeature(DataStructure& dataStructure, const std::atomic_bool& shoul args.insertOrAssign(CropImageGeometryFilter::k_MaxVoxel_Key, std::make_any>(maxVoxels)); args.insertOrAssign(CropImageGeometryFilter::k_CreatedImageGeometryPath_Key, std::make_any(createdImgGeomPath)); + const std::string boundsText = fmt::format("voxels [{}, {}, {}] to [{}, {}, {}]", minVoxels[0], minVoxels[1], minVoxels[2], maxVoxels[0], maxVoxels[1], maxVoxels[2]); + auto preflightResult = filter.preflight(dataStructure, args); if(preflightResult.outputActions.invalid()) { - return MakeErrorResult(k_CropPreflightError, fmt::format("Preflight of the crop that extracts feature geometry '{}' (voxels [{}, {}, {}] to [{}, {}, {}]) from '{}' failed: {}", - createdImgGeomPath.toString(), minVoxels[0], minVoxels[1], minVoxels[2], maxVoxels[0], maxVoxels[1], maxVoxels[2], + return MakeErrorResult(k_CropPreflightError, fmt::format("Preflight of the crop that extracts feature geometry '{}' ({}) from '{}' failed: {}", createdImgGeomPath.toString(), boundsText, imageGeometryPath.toString(), FirstErrorMessage(preflightResult.outputActions.errors()))); } @@ -58,8 +84,8 @@ Result<> CropFeature(DataStructure& dataStructure, const std::atomic_bool& shoul auto executeResult = filter.execute(dataStructure, args); if(executeResult.result.invalid()) { - return MakeErrorResult(k_CropExecuteError, fmt::format("The crop that extracts feature geometry '{}' from '{}' failed: {}", createdImgGeomPath.toString(), imageGeometryPath.toString(), - FirstErrorMessage(executeResult.result.errors()))); + return MakeErrorResult(k_CropExecuteError, fmt::format("The crop that extracts feature geometry '{}' ({}) from '{}' failed: {}", createdImgGeomPath.toString(), boundsText, + imageGeometryPath.toString(), FirstErrorMessage(executeResult.result.errors()))); } return {}; } @@ -95,10 +121,9 @@ Result<> RemoveFlaggedFeatures::operator()() flaggedFeatures = MaskCompareUtilities::InstantiateMaskCompare(m_DataStructure, m_InputValues->FlaggedFeaturesArrayPath); } catch(const std::out_of_range& exception) { - // This really should NOT be happening as the path was verified during preflight BUT we may be calling this from - // somewhere else that is NOT going through the normal nx::core::IFilter API of Preflight and Execute - std::string message = fmt::format("Mask Array DataPath does not exist or is not of the correct type (Bool | UInt8) {}", m_InputValues->FlaggedFeaturesArrayPath.toString()); - return MakeErrorResult(k_MaskArrayError, message); + // Preflight verifies the path, but the algorithm can also be run directly without preflight. + return MakeErrorResult(k_MaskArrayError, + fmt::format("The flagged features array at path '{}' does not exist or is not of type Bool or UInt8.", m_InputValues->FlaggedFeaturesArrayPath.toString())); } if(getCancel()) @@ -106,7 +131,7 @@ Result<> RemoveFlaggedFeatures::operator()() return {}; } - Result<> result; + Result<> extractWarnings; // Valid values Functionality::Extract and Functionality::ExtractThenRemove if(function != Functionality::Remove) @@ -136,12 +161,25 @@ Result<> RemoveFlaggedFeatures::operator()() auto executeResult = filter.execute(m_DataStructure, args); if(executeResult.result.invalid()) { + m_DataStructure.removeData(m_InputValues->TempBoundsPath); return MakeErrorResult(k_FeatureRectExecuteError, fmt::format("The feature bounding-box computation for Feature Ids array '{}' failed: {}", m_InputValues->FeatureIdsArrayPath.toString(), FirstErrorMessage(executeResult.result.errors()))); } } - auto bounds = m_DataStructure.getDataRefAs(m_InputValues->TempBoundsPath); + // Copy the six bounds per feature out of the temporary array, then delete it at once. Deleting + // it here keeps it out of every extracted geometry (the crop copies the whole feature Attribute + // Matrix) and out of the DataStructure on every later error return. + std::vector bounds; + { + const auto& boundsStore = m_DataStructure.getDataRefAs(m_InputValues->TempBoundsPath).getDataStoreRef(); + bounds.resize(boundsStore.getSize()); + for(usize i = 0; i < bounds.size(); i++) + { + bounds[i] = boundsStore[i]; + } + } + m_DataStructure.removeData(m_InputValues->TempBoundsPath); if(getCancel()) { @@ -150,9 +188,10 @@ Result<> RemoveFlaggedFeatures::operator()() // Each crop adds a geometry to the DataStructure, which is not thread-safe, so the features are // extracted one at a time. - usize maxTuple = flaggedFeatures->getNumberOfTuples(); - std::string paddingWidth = std::to_string(std::to_string(maxTuple).size()); - for(usize i = 1; i < maxTuple; i++) + const usize maxTuple = flaggedFeatures->getNumberOfTuples(); + const std::string paddingWidth = std::to_string(std::to_string(maxTuple).size()); + std::vector emptyFeatures; + for(usize i = 1; i < maxTuple && 6 * i + 5 < bounds.size(); i++) { if(getCancel()) { @@ -164,7 +203,7 @@ Result<> RemoveFlaggedFeatures::operator()() continue; } - usize index = 6 * i; + const usize index = 6 * i; std::vector minVoxels = {static_cast(bounds[index]), static_cast(bounds[index + 1]), static_cast(bounds[index + 2])}; std::vector maxVoxels = {static_cast(bounds[index + 3]), static_cast(bounds[index + 4]), static_cast(bounds[index + 5])}; @@ -172,9 +211,7 @@ Result<> RemoveFlaggedFeatures::operator()() // feature that owns no cell is left with minimum > maximum. There is nothing to crop for it. if(minVoxels[0] > maxVoxels[0] || minVoxels[1] > maxVoxels[1] || minVoxels[2] > maxVoxels[2]) { - result.warnings().push_back( - Warning{k_EmptyFeatureSkippedWarning, fmt::format("Feature {} is flagged for extraction but owns no cell in the Feature Ids array '{}'. No geometry was created for it.", i, - m_InputValues->FeatureIdsArrayPath.toString())}); + emptyFeatures.push_back(i); continue; } @@ -184,8 +221,24 @@ Result<> RemoveFlaggedFeatures::operator()() Result<> cropResult = CropFeature(m_DataStructure, getCancel(), m_InputValues->ImageGeometryPath, minVoxels, maxVoxels, createdImgGeomPath); if(cropResult.invalid()) { - return MergeResults(std::move(result), std::move(cropResult)); + return MergeResults(std::move(extractWarnings), std::move(cropResult)); + } + } + + if(!emptyFeatures.empty()) + { + std::string listed; + for(usize k = 0; k < std::min(emptyFeatures.size(), k_MaxListedEmptyFeatures); k++) + { + listed += fmt::format("{}{}", k == 0 ? "" : ", ", emptyFeatures[k]); + } + if(emptyFeatures.size() > k_MaxListedEmptyFeatures) + { + listed += ", ..."; } + extractWarnings.warnings().push_back(Warning{k_EmptyFeatureSkippedWarning, fmt::format("{} flagged feature(s) own no cell in the Feature Ids array '{}' and were skipped; no geometry was " + "created for them. Feature id(s): {}", + emptyFeatures.size(), m_InputValues->FeatureIdsArrayPath.toString(), listed)}); } m_MessageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("All Features Successfully Extracted")}); @@ -215,8 +268,8 @@ Result<> RemoveFlaggedFeatures::operator()() removalArgs.FillRemovedFeatures = m_InputValues->FillRemovedFeatures; Result<> removeResult = FeatureRemovalUtilities::removeFlaggedFeatures(m_DataStructure, flagVector, removalArgs, m_MessageHandler, m_ShouldCancel); - return MergeResults(std::move(result), std::move(removeResult)); + return MergeResults(std::move(extractWarnings), std::move(removeResult)); } - return result; + return extractWarnings; } diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp index f9ba982da8..f460ca14d7 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.cpp @@ -5,8 +5,6 @@ #include "simplnx/DataStructure/AttributeMatrix.hpp" #include "simplnx/DataStructure/DataArray.hpp" #include "simplnx/DataStructure/DataPath.hpp" -#include "simplnx/Filter/Actions/CreateArrayAction.hpp" -#include "simplnx/Filter/Actions/DeleteDataAction.hpp" #include "simplnx/Parameters/ArraySelectionParameter.hpp" #include "simplnx/Parameters/BoolParameter.hpp" #include "simplnx/Parameters/ChoicesParameter.hpp" @@ -21,8 +19,10 @@ using namespace nx::core; namespace { +// Name of the per-feature bounding-box array that the extract path creates in the feature Attribute Matrix +// through ComputeFeatureRectFilter and deletes again before the first crop. const std::string k_boundsName = "tempBounds"; -} +} // namespace namespace nx::core { @@ -130,17 +130,6 @@ IFilter::PreflightResult RemoveFlaggedFeaturesFilter::preflightImpl(const DataSt -9892, fmt::format("Could not find the parent Attribute Matrix for the selected Flagged Features Data Array at path '{}'", pFlaggedFeaturesArrayPathValue.toString()))}; } - auto pFunctionality = filterArgs.value(k_Functionality_Key); - if(pFunctionality != to_underlying(Functionality::Remove)) - { - DataPath const tempPath = pFlaggedFeaturesArrayPathValue.replaceName(k_boundsName); - auto action = - std::make_unique(DataType::uint32, std::vector{featureIdsArray.getNumberOfTuples()}, std::vector{featureIdsArray.getNumberOfComponents() * 6}, tempPath); - - // After the execute function has been done, delete the temp array - resultOutputActions.value().appendDeferredAction(std::make_unique(tempPath)); - } - // If we are in any way removing features, inform the user if(operationType == 0 || operationType == 2) { diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp index e4fa593b06..3d4a49ac43 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp @@ -8,6 +8,7 @@ #include "simplnx/Utilities/NeighborUtilities.hpp" #include +#include using namespace nx::core; @@ -17,31 +18,34 @@ constexpr int32 k_AllFeaturesFlaggedError = -45433; constexpr int32 k_RemoveInactiveObjectsError = -45434; constexpr int32 k_FeatureIdOutOfRangeError = -45435; constexpr int32 k_NoFillProgressError = -45436; +constexpr int32 k_TupleCountMismatchError = -45437; +constexpr int32 k_FeatureIdsCannotBeIgnoredWarning = -45438; -/** - * @brief Outcome of one pass of IdentifyNeighbors() over the volume. - */ -struct NeighborScan -{ - /// At least one cell still holds a negative (vacated) FeatureId. - bool unresolvedCellsRemain = false; - /// At least one vacated cell has a non-negative face neighbor recorded as its fill source. - bool fillSourceFound = false; -}; +/// Number of cells between cancel polls in the validation pass. +constexpr usize k_CancelPollStride = 1ULL << 20; /** * @brief Chooses a fill source for every vacated cell. * * A vacated cell is one whose FeatureId is negative. Its six face neighbors are polled in the order * -Z, -Y, -X, +X, +Y, +Z. Every non-negative neighbor FeatureId is tallied, background (0) included, - * and the source becomes the neighbor whose feature first reaches the highest tally. A cell with no - * non-negative neighbor gets no source this pass and is retried on the next pass, after its own - * neighbors have been filled. + * and the source becomes the neighbor whose feature first reaches the highest tally. The first + * sighting counts as one hit, so a cell whose valid neighbors all belong to distinct features gets + * a source in this pass. A cell with no non-negative neighbor gets no source this pass and is + * retried on the next pass, after its own neighbors have been filled. + * + * Cells with FeatureId 0 are background. They are never fill targets, so they are not counted as + * unresolved. They are legal fill sources, which matches DREAM3D 6.5.171. * - * Cells with FeatureId 0 are background. They are never fill targets, so they do not keep the caller - * iterating. They are legal fill sources, which matches DREAM3D 6.5.171. + * @param imageGeom Supplies the volume dimensions. + * @param featureIds Cell FeatureIds. Negative values mark vacated cells. Not modified. + * @param storageArray Receives, for each vacated cell, the index of the neighbor cell to copy from. + * Entries for cells that get no source are left unchanged. + * @param shouldCancel Polled once per Z slice. When set the scan returns early. + * @param messageHelper Throttled progress messages. + * @return The number of vacated cells seen in this pass. Zero means the fill is complete. */ -NeighborScan IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& featureIds, std::vector& storageArray, const std::atomic_bool& shouldCancel, MessageHelper& messageHelper) +usize IdentifyNeighbors(const ImageGeom& imageGeom, const Int32AbstractDataStore& featureIds, std::vector& storageArray, const std::atomic_bool& shouldCancel, MessageHelper& messageHelper) { ThrottledMessenger throttledMessenger = messageHelper.createThrottledMessenger(); @@ -57,17 +61,15 @@ NeighborScan IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& fea const std::array neighborVoxelIndexOffsets = initializeFaceNeighborOffsets(dims); constexpr std::array faceNeighborInternalIdx = initializeFaceNeighborInternalIdx(); - NeighborScan scan; + usize unresolvedCellCount = 0; auto progressIncrement = dims[2] / 100; usize progressCounter = 0; - int32 featureName; - int64 kStride, jStride; for(int64 zIdx = 0; zIdx < dims[2]; zIdx++) { if(shouldCancel) { - return scan; + return unresolvedCellCount; } if(progressCounter > progressIncrement) @@ -77,27 +79,28 @@ NeighborScan IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& fea } progressCounter++; - kStride = dims[0] * dims[1] * zIdx; + const int64 kStride = dims[0] * dims[1] * zIdx; for(int64 yIdx = 0; yIdx < dims[1]; yIdx++) { - jStride = dims[0] * yIdx; + const int64 jStride = dims[0] * yIdx; for(int64 xIdx = 0; xIdx < dims[0]; xIdx++) { - int64 voxelIndex = kStride + jStride + xIdx; - featureName = featureIds[voxelIndex]; - // Only vacated cells (negative) need a source. Background (0) is not a fill target; treating - // it as one would keep the caller's loop alive forever because nothing ever overwrites it. + const int64 voxelIndex = kStride + jStride + xIdx; + const int32 featureName = featureIds[voxelIndex]; + // Only vacated cells (negative) need a source. Background (0) is not a fill target; counting + // it as unresolved would keep the caller's loop alive forever because nothing overwrites it. if(featureName >= 0) { continue; } - scan.unresolvedCellsRemain = true; - int32 current; + unresolvedCellCount++; + + // At most six distinct features can be seen, so fixed storage avoids a heap allocation per cell. int32 most = 0; - std::vector numHits(6, 0); - std::vector discoveredFeatures = {}; - discoveredFeatures.reserve(6); - // Loop over the 6 face neighbors of the voxel + std::array numHits{}; + std::array discoveredFeatures{}; + usize discoveredCount = 0; + const std::array isValidFaceNeighbor = computeValidFaceNeighbors(xIdx, yIdx, zIdx, dims); for(const auto& faceIndex : faceNeighborInternalIdx) { @@ -106,61 +109,70 @@ NeighborScan IdentifyNeighbors(ImageGeom& imageGeom, Int32AbstractDataStore& fea continue; } - int64 neighborPoint = voxelIndex + neighborVoxelIndexOffsets[faceIndex]; - int32 feature = featureIds[neighborPoint]; - if(feature >= 0) + const int64 neighborPoint = voxelIndex + neighborVoxelIndexOffsets[faceIndex]; + const int32 feature = featureIds[neighborPoint]; + if(feature < 0) { - bool found = false; - for(usize featIndex = 0; featIndex < discoveredFeatures.size(); featIndex++) - { - if(discoveredFeatures[featIndex] == feature) - { - found = true; - numHits[featIndex]++; - current = numHits[featIndex]; - if(current > most) - { - most = current; - storageArray[voxelIndex] = static_cast(neighborPoint); - scan.fillSourceFound = true; - } - break; - } - } - if(!found) - { - // Count the first sighting as a hit. Without this the tally only records a neighbor on - // the SECOND sighting of a feature, so a bad cell whose valid neighbors all belong to - // distinct features never gets a fill source. It stays negative, and the caller's - // do/while dilation loop never terminates. - discoveredFeatures.push_back(feature); - numHits[discoveredFeatures.size() - 1] = 1; - if(1 > most) - { - most = 1; - storageArray[voxelIndex] = static_cast(neighborPoint); - scan.fillSourceFound = true; - } - } + continue; + } + + usize featIndex = 0; + while(featIndex < discoveredCount && discoveredFeatures[featIndex] != feature) + { + featIndex++; + } + if(featIndex == discoveredCount) + { + discoveredFeatures[discoveredCount] = feature; + numHits[discoveredCount] = 0; + discoveredCount++; + } + numHits[featIndex]++; + if(numHits[featIndex] > most) + { + most = numHits[featIndex]; + storageArray[voxelIndex] = static_cast(neighborPoint); } } } } } - return scan; + return unresolvedCellCount; } /** - * @brief Checks that every FeatureId indexes a tuple of the feature Attribute Matrix. + * @brief Checks the Feature Ids array before anything is modified. + * + * FlagFeatures() indexes a vector of size totalFeatures with each cell's FeatureId, and the fill loop + * indexes the array with cell indices computed from the geometry, so both the value range and the + * tuple count must be right. * - * FlagFeatures() indexes a vector of size totalFeatures with each cell's FeatureId, so a negative or - * too-large value is an out-of-bounds read. Run this before anything is modified. + * @param featureIds Cell array to check. + * @param totalCells Number of cells in the Image Geometry. + * @param totalFeatures Tuple count of the feature Attribute Matrix. + * @param featureIdsPath Named in the error messages. + * @param featureAttributeMatrixPath Named in the error messages. + * @param shouldCancel Polled every k_CancelPollStride cells. + * @return Error -45437 if the tuple count differs from totalCells, error -45435 naming the first + * cell whose value is outside [0, totalFeatures), otherwise valid. */ -Result<> ValidateFeatureIds(const Int32AbstractDataStore& featureIds, usize totalFeatures, const DataPath& featureIdsPath, const DataPath& featureAttributeMatrixPath) +Result<> ValidateFeatureIds(const Int32AbstractDataStore& featureIds, usize totalCells, usize totalFeatures, const DataPath& featureIdsPath, const DataPath& featureAttributeMatrixPath, + const std::atomic_bool& shouldCancel) { const usize totalPoints = featureIds.getNumberOfTuples(); + if(totalPoints != totalCells) + { + return MakeErrorResult(k_TupleCountMismatchError, fmt::format("The Feature Ids array '{}' has {} tuple(s), but the selected Image Geometry has {} cell(s). The array must hold exactly one value " + "per cell of the selected geometry. No data was modified.", + featureIdsPath.toString(), totalPoints, totalCells)); + } + for(usize i = 0; i < totalPoints; i++) { + if(i % k_CancelPollStride == 0 && shouldCancel) + { + return {}; + } const int32 featureId = featureIds[i]; if(featureId < 0 || static_cast(featureId) >= totalFeatures) { @@ -173,6 +185,16 @@ Result<> ValidateFeatureIds(const Int32AbstractDataStore& featureIds, usize tota return {}; } +/** + * @brief Marks the cells of every flagged feature. + * + * @param featureIds Cell FeatureIds, already validated. Cells of flagged features are set to -1 when + * fillRemovedFeatures is true, otherwise to 0. + * @param flaggedFeatures true means remove. Index 0 is ignored. + * @param fillRemovedFeatures Selects the marker value. + * @return One entry per feature, true for features that survive. Empty when every feature is flagged; + * in that case nothing was modified. + */ std::vector FlagFeatures(Int32AbstractDataStore& featureIds, const std::vector& flaggedFeatures, const bool fillRemovedFeatures) { bool good = false; @@ -213,31 +235,43 @@ std::vector FlagFeatures(Int32AbstractDataStore& featureIds, const std::ve return activeObjects; } -void FindVoxelArrays(const Int32AbstractDataStore& featureIds, const std::vector& neighbors, std::vector>& voxelArrays, const std::atomic_bool& shouldCancel) +/** + * @brief Copies every listed cell array from each vacated cell's chosen source cell. + * + * @param featureIds Cell FeatureIds. Read to find vacated cells; it is also one of the voxelArrays and + * is overwritten through that list. + * @param neighbors Source cell index per cell, or -1 when the cell has no source this pass. + * @param voxelArrays Every cell array to copy, including the Feature Ids array itself. + * @param shouldCancel Polled once per cell. + * @return The number of cells that received a copy. Zero means the pass made no progress. + */ +usize FindVoxelArrays(const Int32AbstractDataStore& featureIds, const std::vector& neighbors, std::vector>& voxelArrays, const std::atomic_bool& shouldCancel) { const usize totalPoints = featureIds.getNumberOfTuples(); + usize filledCellCount = 0; - int32 featureName, neighbor; for(usize j = 0; j < totalPoints; j++) { if(shouldCancel) { - return; + return filledCellCount; } - featureName = featureIds[j]; - neighbor = neighbors[j]; - if(neighbor >= 0) + const int32 neighbor = neighbors[j]; + if(neighbor < 0) + { + continue; + } + if(featureIds[j] < 0 && featureIds[neighbor] >= 0) { - if(featureName < 0 && featureIds[neighbor] >= 0) + for(const auto& voxelArray : voxelArrays) { - for(const auto& voxelArray : voxelArrays) - { - voxelArray->copyTuple(neighbor, j); - } + voxelArray->copyTuple(neighbor, j); } + filledCellCount++; } } + return filledCellCount; } } // namespace @@ -251,10 +285,19 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector(args.FeatureIdsArrayPath)->getDataStoreRef(); MessageHelper messageHelper(messageHandler); + Result<> result; messageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("Beginning Feature Removal")}); - Result<> validation = ValidateFeatureIds(featureIds, flaggedFeatures.size(), args.FeatureIdsArrayPath, args.FeatureAttributeMatrixPath); + // Tuple 0 is the unused feature, so fewer than two tuples means there is no feature to keep. + if(flaggedFeatures.size() < 2) + { + return MakeErrorResult(k_AllFeaturesFlaggedError, fmt::format("The feature Attribute Matrix '{}' has {} tuple(s). Tuple 0 is the unused feature, so at least 2 tuples are required for a " + "feature to survive the removal. No data was modified.", + args.FeatureAttributeMatrixPath.toString(), flaggedFeatures.size())); + } + + Result<> validation = ValidateFeatureIds(featureIds, imageGeom.getNumberOfCells(), flaggedFeatures.size(), args.FeatureIdsArrayPath, args.FeatureAttributeMatrixPath, shouldCancel); if(validation.invalid()) { return validation; @@ -279,42 +322,66 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector ignoredPaths; + ignoredPaths.reserve(args.IgnoredDataArrayPaths.size()); + for(const DataPath& path : args.IgnoredDataArrayPaths) + { + if(path == args.FeatureIdsArrayPath) + { + result.warnings().push_back(Warning{k_FeatureIdsCannotBeIgnoredWarning, fmt::format("The Feature Ids array '{}' was listed in the arrays to ignore. It is the array being filled and " + "cannot be ignored, so it was removed from the list.", + path.toString())}); + continue; + } + ignoredPaths.push_back(path); + } + // The set of cell arrays does not change during the fill, so build the list once. + std::vector> voxelArrays = GenerateDataArrayList(dataStructure, args.FeatureIdsArrayPath, ignoredPaths); + // One int32 per cell. Declared here rather than at function scope because only the fill path // uses it; hoisting it would cost a 4 GB allocation on a 1000^3 volume even with fill disabled, // which is the default. std::vector neighbors((featureIds.getNumberOfTuples() * featureIds.getNumberOfComponents()), -1); - bool shouldLoop; usize count = 0; - do + while(true) { count++; messageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("Entering iteration number {}...", count)}); std::fill(neighbors.begin(), neighbors.end(), -1); - const NeighborScan scan = IdentifyNeighbors(imageGeom, featureIds, neighbors, shouldCancel, messageHelper); + const usize unresolvedCellCount = IdentifyNeighbors(imageGeom, featureIds, neighbors, shouldCancel, messageHelper); if(shouldCancel) { return {}; } + if(unresolvedCellCount == 0) + { + break; + } - // Vacated cells remain but none of them touches a cell that belongs to a feature. Another pass - // would find the same state, so stop instead of looping forever. This only happens when every - // cell belonged to a flagged feature and the unflagged features own no cells. - if(scan.unresolvedCellsRemain && !scan.fillSourceFound) + messageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("Filling {} bad voxels...", unresolvedCellCount)}); + const usize filledCellCount = FindVoxelArrays(featureIds, neighbors, voxelArrays, shouldCancel); + + if(shouldCancel) { - return MakeErrorResult(k_NoFillProgressError, - fmt::format("Fill iteration {} could not fill any of the remaining vacated cells in the Feature Ids array '{}' because none of them has a face neighbor that belongs " - "to a surviving feature. This happens when every cell belongs to a flagged feature and the unflagged feature(s) own no cells. Unflag a feature that " - "owns cells, or disable 'Fill-in Removed Features'. THE FOLLOWING ARRAY HAS BEEN MODIFIED: '{}' (removed cells are set to -1).", - count, args.FeatureIdsArrayPath.toString(), args.FeatureIdsArrayPath.toString())); + return {}; } - messageHandler(IFilter::ProgressMessage{IFilter::Message::Type::Info, fmt::format("Filling bad voxels...")}); - std::vector> voxelArrays = GenerateDataArrayList(dataStructure, args.FeatureIdsArrayPath, args.IgnoredDataArrayPaths); - FindVoxelArrays(featureIds, neighbors, voxelArrays, shouldCancel); - shouldLoop = scan.unresolvedCellsRemain; - } while(shouldLoop); + // A pass that fills nothing would be repeated with the same result, so stop instead of looping + // forever. This happens when every cell belonged to a flagged feature and the unflagged + // features own no cells, so no vacated cell touches a cell that belongs to a feature. + if(filledCellCount == 0) + { + return MakeErrorResult(k_NoFillProgressError, + fmt::format("Fill iteration {} could not fill any of the {} remaining vacated cell(s) in the Feature Ids array '{}' because none of them has a face neighbor that " + "belongs to a surviving feature. This happens when every cell belongs to a flagged feature and the unflagged feature(s) own no cells. Unflag a " + "feature that owns cells, or disable 'Fill-in Removed Features'. THE FOLLOWING ARRAY HAS BEEN MODIFIED: '{}' (removed cells are set to -1).", + count, unresolvedCellCount, args.FeatureIdsArrayPath.toString(), args.FeatureIdsArrayPath.toString())); + } + } } if(shouldCancel) @@ -328,6 +395,6 @@ Result<> removeFlaggedFeatures(DataStructure& dataStructure, const std::vector removeFlaggedFeatures(DataStructure& dataStructure, const std::vector& flaggedFeatures, const RemovalArgs& args, const IFilter::MessageHandler& messageHandler, const std::atomic_bool& shouldCancel); diff --git a/src/Plugins/SimplnxCore/test/KeepRemoveRankedFeaturesTest.cpp b/src/Plugins/SimplnxCore/test/KeepRemoveRankedFeaturesTest.cpp index 86fbe0d3cb..5aff09f016 100644 --- a/src/Plugins/SimplnxCore/test/KeepRemoveRankedFeaturesTest.cpp +++ b/src/Plugins/SimplnxCore/test/KeepRemoveRankedFeaturesTest.cpp @@ -430,6 +430,30 @@ TEST_CASE("SimplnxCore::KeepRemoveRankedFeaturesFilter: Ties and non-finite valu SIMPLNX_RESULT_REQUIRE_INVALID(executeResult.result); } + SECTION("Fill terminates when the input has background cells") + { + // Cell 0 becomes background (FeatureId 0). Background is never a fill target, so it must stay 0 + // and the shared fill loop must still terminate. Before the shared utility skipped background + // cells this section ran forever. + DataStructure dataStructure = BuildTestData(); + REQUIRE_NOTHROW(dataStructure.getDataRefAs(k_FeatureIdsPath)); + dataStructure.getDataRefAs(k_FeatureIdsPath).getDataStoreRef()[0] = 0; + Arguments args = MakeArgs(0ULL, 0ULL, 2ULL); + args.insertOrAssign(KeepRemoveRankedFeaturesFilter::k_FillRemovedFeatures_Key, std::make_any(true)); + + auto executeResult = filter.execute(dataStructure, args); + SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result); + + const std::vector featureIds = ReadFeatureIds(dataStructure); + REQUIRE(featureIds[0] == 0); + for(usize i = 1; i < featureIds.size(); i++) + { + REQUIRE(featureIds[i] > 0); + } + + UnitTest::CheckArraysInheritTupleDims(dataStructure); + } + SECTION("Fill leaves no Cell at zero") { DataStructure dataStructure = BuildTestData(); diff --git a/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp b/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp index 19322c8341..49adfbfb69 100644 --- a/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp +++ b/src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp @@ -1,6 +1,8 @@ +#include "SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.hpp" #include "SimplnxCore/Filters/RemoveFlaggedFeaturesFilter.hpp" #include "SimplnxCore/SimplnxCore_test_dirs.hpp" +#include "simplnx/Common/TypeTraits.hpp" #include "simplnx/Core/Application.hpp" #include "simplnx/DataStructure/DataGroup.hpp" #include "simplnx/DataStructure/Geometry/ImageGeom.hpp" @@ -18,8 +20,6 @@ #include #include #include -#include -#include #include #include @@ -38,10 +38,13 @@ namespace fs = std::filesystem; * CellValue - 100 + cell index. Unique per cell, so after a fill the value in a vacated cell * identifies exactly WHICH neighbor cell it was copied from. * IgnoredValue - 500 + cell index. Passed as an ignored array and must never change. - * and one Feature array, Int32DataSet = 1000 + feature id, so the compaction can be checked tuple-by-tuple. + * One Feature array, Int32DataSet = 1000 + feature id, lets the compaction be checked tuple-by-tuple. * - * Fill semantics under test (shared with RequireMinNumNeighbors and Keep/Remove Ranked Features via - * FeatureRemovalUtilities): every vacated cell (FeatureId -1) polls its six face neighbors in the order + * Fixtures are lettered A, B and E to match the legacy A/B comparison set, where C, D and F are the + * error-path fixtures that appear here under their error names. + * + * Fill semantics under test (shared with Keep/Remove Ranked Features via FeatureRemovalUtilities): + * every vacated cell (FeatureId -1) polls its six face neighbors in the order * -Z, -Y, -X, +X, +Y, +Z, tallies each non-negative neighbor FeatureId (0 counts), and copies the Cell * tuple of the neighbor whose feature reached the highest tally first. Ties resolve to the feature seen * first in that order. Cells with no non-negative neighbor wait for the next iteration. @@ -61,13 +64,15 @@ const DataPath k_FeatureAMPath({k_DataContainer, k_CellFeatureData}); const DataPath k_FlaggedFeaturesPath({k_DataContainer, k_CellFeatureData, k_ActiveName}); const DataPath k_FeatureValuePath({k_DataContainer, k_CellFeatureData, k_Int32DataSet}); -constexpr uint64 k_Remove = 0; -constexpr uint64 k_Extract = 1; -constexpr uint64 k_ExtractThenRemove = 2; +constexpr uint64 k_Remove = to_underlying(Functionality::Remove); +constexpr uint64 k_Extract = to_underlying(Functionality::Extract); +constexpr uint64 k_ExtractThenRemove = to_underlying(Functionality::ExtractThenRemove); constexpr int32 k_AllFeaturesFlaggedError = -45433; constexpr int32 k_FeatureIdOutOfRangeError = -45435; constexpr int32 k_NoFillProgressError = -45436; +constexpr int32 k_TupleCountMismatchError = -45437; +constexpr int32 k_FeatureIdsCannotBeIgnoredWarning = -45438; constexpr int32 k_ParentNotAttributeMatrixError = -9892; constexpr int32 k_NeighborListRemovalWarning = -5558; constexpr int32 k_EmptyFeatureSkippedWarning = -53905; @@ -107,6 +112,7 @@ void BuildFixture(DataStructure& dataStructure, const FixtureSpec& spec) auto& ignoredValue = UnitTest::CreateTestDataArray(dataStructure, k_IgnoredValueName, tupleDims, {1}, cellAM->getId())->getDataStoreRef(); for(usize i = 0; i < spec.featureIds.size(); i++) { + // Cell values are unique per cell so a fill copy is traceable to its source cell. featureIds[i] = spec.featureIds[i]; cellValue[i] = static_cast(100 + i); ignoredValue[i] = static_cast(500 + i); @@ -182,7 +188,7 @@ int32 RunExecuteError(DataStructure& dataStructure, const Arguments& args) bool HasWarningCode(const std::vector& warnings, int32 code) { - return std::any_of(warnings.begin(), warnings.end(), [code](const Warning& w) { return w.code == code; }); + return std::any_of(warnings.begin(), warnings.end(), [code](const Warning& warning) { return warning.code == code; }); } // --------------------------------------------------------------------------------------------------- @@ -335,6 +341,28 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill copie UnitTest::CheckArraysInheritTupleDims(dataStructure); } +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Fill cannot ignore the Feature Ids array (-45438 warning)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + // Listing FeatureIds among the ignored arrays used to leave every vacated cell at -1 forever, because + // sources were chosen but the FeatureIds array was never copied. The array is now always copied. + auto executeResult = RunValid(dataStructure, MakeArgs(k_Remove, true, {k_FeatureIdsPath, k_IgnoredValuePath})); + REQUIRE(executeResult.result.warnings().size() == 1); + REQUIRE(executeResult.result.warnings()[0].code == k_FeatureIdsCannotBeIgnoredWarning); + + // Same result as the issue #1698 test. + REQUIRE(ReadInt32(dataStructure, k_FeatureIdsPath) == std::vector{1, 1, 1, 1, 1, 1, 2, 2, 2, 2}); + std::vector expectedCellValue = Sequence(100, 10); + expectedCellValue[3] = 102; + expectedCellValue[9] = 108; + expectedCellValue[4] = 102; + REQUIRE(ReadInt32(dataStructure, k_CellValuePath) == expectedCellValue); + REQUIRE(ReadInt32(dataStructure, k_IgnoredValuePath) == Sequence(500, 10)); + UnitTest::CheckArraysInheritTupleDims(dataStructure); +} + TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill treats FeatureId 0 as a source and never a target", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { UnitTest::LoadPlugins(); @@ -452,6 +480,25 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - FeatureId o } } +TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - Feature Ids tuple count differs from the geometry (-45437)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") +{ + UnitTest::LoadPlugins(); + DataStructure dataStructure; + BuildFixture(dataStructure, FixtureA()); + // A 4-tuple int32 array outside the geometry (an Attribute Matrix rejects a child with the wrong + // tuple shape, so the mismatch can only come from an array that lives elsewhere). + auto* otherGroup = DataGroup::Create(dataStructure, "OtherData"); + const DataPath shortIdsPath({"OtherData", "ShortIds"}); + auto* shortIdsArray = UnitTest::CreateTestDataArray(dataStructure, "ShortIds", {4}, {1}, otherGroup->getId()); + REQUIRE(shortIdsArray != nullptr); + shortIdsArray->fill(1); + + Arguments args = MakeArgs(k_Remove, false); + args.insertOrAssign(RemoveFlaggedFeaturesFilter::k_CellFeatureIdsArrayPath_Key, std::make_any(shortIdsPath)); + REQUIRE(RunExecuteError(dataStructure, args) == k_TupleCountMismatchError); + REQUIRE(ReadInt32(dataStructure, shortIdsPath) == std::vector{1, 1, 1, 1}); +} + TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - no fill progress (-45436)", "[SimplnxCore][RemoveFlaggedFeaturesFilter]") { UnitTest::LoadPlugins(); @@ -498,8 +545,9 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract cr REQUIRE(newFeatureAM.getNumberOfTuples() == 4); REQUIRE(ReadInt32(dataStructure, newGeomPath.createChildPath(k_CellFeatureData).createChildPath(k_Int32DataSet)) == Sequence(1000, 4)); - // The temporary bounds array is deleted after execution. + // The temporary bounds array is deleted, and it was not copied into the extracted geometry. REQUIRE(dataStructure.getData(k_FeatureAMPath.createChildPath("tempBounds")) == nullptr); + REQUIRE(dataStructure.getData(newGeomPath.createChildPath(k_CellFeatureData).createChildPath("tempBounds")) == nullptr); UnitTest::CheckArraysInheritTupleDims(dataStructure); } @@ -534,17 +582,22 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Extract - flagged feature w { UnitTest::LoadPlugins(); DataStructure dataStructure; - // Feature 4 exists in the Feature Attribute Matrix but owns no cell. + // Features 4 and 5 exist in the Feature Attribute Matrix but own no cell. FixtureSpec spec = FixtureB(); - spec.numFeatures = 5; - spec.flagged = {false, false, false, true, true}; + spec.numFeatures = 6; + spec.flagged = {false, false, false, true, true, true}; BuildFixture(dataStructure, spec); auto executeResult = RunValid(dataStructure, MakeArgs(k_Extract, false)); - REQUIRE(HasWarningCode(executeResult.result.warnings(), k_EmptyFeatureSkippedWarning)); + // One aggregated warning that names both features, not one warning per feature. + REQUIRE(executeResult.result.warnings().size() == 1); + REQUIRE(executeResult.result.warnings()[0].code == k_EmptyFeatureSkippedWarning); + REQUIRE(executeResult.result.warnings()[0].message.find("2 flagged feature(s)") != std::string::npos); + REQUIRE(executeResult.result.warnings()[0].message.find("4, 5") != std::string::npos); - // Feature 3 is still extracted; feature 4 produces no geometry. + // Feature 3 is still extracted; features 4 and 5 produce no geometry. REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-3"})) != nullptr); REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-4"})) == nullptr); + REQUIRE(dataStructure.getData(DataPath({k_NewImgGeomPrefix + "-5"})) == nullptr); UnitTest::CheckArraysInheritTupleDims(dataStructure); } @@ -789,6 +842,7 @@ TEST_CASE("SimplnxCore::RemoveFlaggedFeaturesFilter: Class 4 Invariants - Small REQUIRE(phaseMismatch == 0); REQUIRE(filledWithoutMatchingNeighbor == 0); REQUIRE(untouchedRenumberedWrong == 0); + // Snapshot of 6_5_test_data_1_v2: the cell count of the 275 flagged features. Changes only if the archive changes. REQUIRE(numFilled == 8535); // I6: surviving features only grow, and the growth accounts for every vacated cell. diff --git a/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md index a79574ec7e..c6359f54ab 100644 --- a/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md @@ -14,17 +14,17 @@ | Aspect | Current state | |------------------------|--------------------------| | Algorithm Relationship | **Minor changes** - SIMPLNX merges legacy `RemoveFlaggedFeatures` (Processing) and `ExtractFlaggedFeatures` (Sampling) into one filter with an operation selector. The removal and fill loop is a port that now lives in the shared `FeatureRemovalUtilities`; the extract path is rebuilt on the `ComputeFeatureRect` and `CropImageGeometry` sub-filters. | -| Oracle (confirmed) | **Class 1 analytical + Class 4 invariants** - three hand-derived fixtures (5x2x1, 4x4x1, 3x3x3) pin every filled cell to the exact neighbor it was copied from, plus a Small IN100 invariant test that fills 8,535 cells. Encoded in `test/RemoveFlaggedFeaturesTest.cpp`; all 16 test cases pass in the in-core and out-of-core builds. | -| Code paths enumerated | 20 of 28 paths are assertion-covered. The 8 uncovered paths are cancellation and defensive sub-filter failure paths. | -| Tests today | 16 test cases: 8 Class 1 analytical (remove, fill, extract, extract-then-remove), 3 execute-error, 1 preflight-error, 1 preflight-warning, 1 Class 4 Small IN100 invariant, 1 empty-feature warning, 1 SIMPL 6.4/6.5 conversion. The 3 original tests are retired. | +| Oracle (confirmed) | **Class 1 analytical + Class 4 invariants** - three hand-derived fixtures (5x2x1, 4x4x1, 3x3x3) pin every filled cell to the exact neighbor it was copied from, plus a Small IN100 invariant test that fills 8,535 cells. Encoded in `test/RemoveFlaggedFeaturesTest.cpp`; all 18 test cases pass in `simplnx-Rel` and `simplnx-ooc-Rel`. | +| Code paths enumerated | 23 of 31 paths are assertion-covered. The 8 uncovered paths are cancellation and defensive sub-filter failure paths. | +| Tests today | 18 test cases: 8 Class 1 analytical (remove, fill, extract, extract-then-remove), 4 execute-error, 1 preflight-error, 1 preflight-warning, 1 fill-ignore-list warning, 1 empty-feature warning, 1 Class 4 Small IN100 invariant, 1 SIMPL 6.4/6.5 conversion. The 3 original tests are retired. **OOC caveat:** the `simplnx-ooc-Rel` preset registers no OOC backend, so that pass certifies compile and run in that configuration, not out-of-core data-path behavior. | | Exemplar archive | No output archive. The inline oracle needs none. The Class 4 test reads the shared input archive `6_5_test_data_1_v2.tar.gz` (SHA512 `585b51ba...3027d6c`, sidecar exists). | -| Legacy comparison | **Run (2026-09-02)** - DREAM3D 6.5.171 and SIMPLNX were run on the 5x2x1, 4x4x1 and 3x3x3 fixtures with fill on and off, and on the 4x4x1 extract. Every removal array (25 array comparisons) matched the oracle and each other exactly. The extract geometry, origin and cell arrays matched. Seven deviations are documented; five are bugs, four of which were in SIMPLNX. | +| Legacy comparison | **Run (2026-09-02)** - DREAM3D 6.5.171 and SIMPLNX were run on the 5x2x1, 4x4x1 and 3x3x3 fixtures with fill on and off, and on the 4x4x1 extract. Every removal array (25 array comparisons) matched the oracle and each other exactly. The extract geometry, origin and cell arrays matched. Seven deviations are documented. Six are bugs, all present in SIMPLNX before this branch and three (D3, D4, D6) shared with 6.5.171. D7 is an algorithmic choice. | | Bug flags | `RemoveFlaggedFeaturesFilter-D1` through `-D6`. D1 and D2 are hangs that only SIMPLNX had; D3 and D4 are shared with 6.5.171; D5 is SIMPLNX only; D6 affects both differently. All are fixed in SIMPLNX on this branch (D1 was fixed in v7.4.2). | -| V&V phase | Discovery, oracle, reconciliation, algorithm review, tests, dual-build pass, legacy comparison, deviations and documentation are complete. Outstanding: second-engineer review of the oracle and this report at PR review. | +| V&V phase | Discovery, oracle, reconciliation, algorithm review, tests, dual-build pass, legacy comparison, deviations and documentation are complete. Five scoped review passes (adversarial, senior engineer, CPU, memory, out-of-core) were run on the change set and their findings applied; see *Review findings*. Outstanding: second-engineer review of the oracle and this report at PR review. | ## Summary -`RemoveFlaggedFeaturesFilter` removes or extracts the **Features** marked true in a boolean feature array, optionally filling vacated **Cells** by iterative dilation from face neighbors, then compacts the feature Attribute Matrix. Verification uses three Class 1 analytical fixtures whose per-cell copy sources are hand-derived, a Class 4 Small IN100 invariant test, error and preflight tests, and SIMPL conversion checks. The removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the V&V found and fixed two SIMPLNX-only infinite loops (background cells, and distinct first-sighting neighbors as reported in issue #1698), added guards for two failure modes shared with 6.5.171 (no fill progress, out-of-range FeatureId), and fixed the extract path's swallowed sub-filter errors and its crash on a flagged feature with no cells. +`RemoveFlaggedFeaturesFilter` removes or extracts the **Features** marked true in a boolean feature array, optionally filling vacated **Cells** by iterative dilation from face neighbors, then compacts the feature Attribute Matrix. Verification uses three Class 1 analytical fixtures whose per-cell copy sources are hand-derived, a Class 4 Small IN100 invariant test, error and preflight tests, and SIMPL conversion checks. The removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the V&V found and fixed two SIMPLNX-only infinite loops (background cells, and distinct first-sighting neighbors as reported in issue #1698), added guards for failure modes shared with 6.5.171 (no fill progress, including the Feature Ids array listed as ignored; out-of-range FeatureId; tuple count that does not match the geometry), and fixed the extract path's swallowed sub-filter errors, its crash on a flagged feature with no cells, and the copy of its temporary bounds array into every extracted geometry. ## Algorithm Relationship @@ -37,13 +37,13 @@ 1. Face-neighbor bookkeeping uses the shared `NeighborUtilities` helpers instead of six inline boundary tests. The traversal order (-Z, -Y, -X, +X, +Y, +Z) and the boundary rules are the same, so the output is unchanged. Confirmed by the bit-identical A/B. 2. The neighbor tally is a per-cell `discoveredFeatures`/`numHits` pair instead of the legacy feature-indexed `n[]` vector. Before PR #1700 the SIMPLNX tally did not count the first sighting, which changed output by never filling some cells (D1). With the first sighting counted, the tally is equivalent to legacy. 3. The vacated-cell test was `featureName > 0` instead of the legacy `featurename < 0`, which made background cells (FeatureId 0) permanent loop targets (D2). This branch restores `>= 0 -> skip`, matching legacy. -4. Two guards with no legacy counterpart were added on this branch: FeatureId range validation before any modification (D4) and a no-progress check on the fill loop (D3). Both turn undefined behavior or an infinite loop into a deterministic error and do not change valid output. +4. Guards with no legacy counterpart were added on this branch: FeatureId range and tuple-count validation before any modification (D4), a no-progress check that counts the cells each fill pass actually fills (D3), and the Feature Ids array is always copied even when listed in the ignore list (D3). All turn undefined behavior or an infinite loop into a deterministic error or warning and do not change valid output. 5. The per-cell neighbor buffer is allocated only when fill is enabled (PR #1700). No output effect. 6. Progress messages are throttled (PR #1340). No output effect. *Port-time deltas (extract path):* -7. Bounds come from `ComputeFeatureRectFilter` (min initialized to `UINT32_MAX`, max to 0) instead of the legacy `-1` sentinel. For a flagged feature with no cells the legacy filter crops a bogus 1x1x1 geometry at origin (-1, -1, -1) (D6); SIMPLNX now warns and skips the feature. +7. Bounds come from `ComputeFeatureRectFilter` (min initialized to `UINT32_MAX`, max to 0) instead of the legacy `-1` sentinel. For a flagged feature with no cells the legacy filter crops a bogus 1x1x1 geometry at origin (-1, -1, -1) (D6); SIMPLNX now warns once, listing the skipped ids, and creates nothing for them. The temporary bounds array is copied into a local buffer and deleted before the first crop, so it is neither carried into the extracted geometries nor left behind on an error return. 8. The extracted geometry is named `-` and carries the feature Attribute Matrix. Legacy names it `Feature_` and copies only cell data (D7). 9. Sub-filter preflight and execute results were tested against the wrong variable and failures threw `std::runtime_error` (D5). This branch returns error results `-53901` through `-53904`. @@ -76,46 +76,59 @@ Expected values are derived by hand from the fixture definitions and the documen |-----------|--------|----------------------------|---------------------------| | `RemoveFlaggedFeaturesFilter-D1` | With *Fill-in Removed Features* on, the neighbor tally only recorded a fill source on the second sighting of a feature, so a vacated cell whose valid neighbors were all distinct features, or which had a single valid neighbor, was never filled and the dilation loop never terminated (GitHub issue #1698). | DREAM3D-NX v7.0.0 through v7.4.1. DREAM.3D 6.5.171 was not affected. | Fixed in v7.4.2 by PR #1700 (first sighting counts as a hit). This branch adds the `Fill from distinct first-sighting neighbors (issue #1698)` regression test that reproduces the issue's 5x2x1 case. | | `RemoveFlaggedFeaturesFilter-D2` | With fill on, any cell with FeatureId 0 was treated as an unresolved cell but never filled, so the dilation loop never terminated on any input that contained background cells. | DREAM3D-NX v7.0.0 through v7.4.2. DREAM.3D 6.5.171 was not affected. | The vacated-cell test is `featureName >= 0 -> skip`, matching 6.5.171. Pinned by `Fill treats FeatureId 0 as a source and never a target`, which hung before the fix. | -| `RemoveFlaggedFeaturesFilter-D3` | With fill on, when every cell belonged to a flagged feature and the only unflagged features owned no cells, no vacated cell had a fill source and the loop spun forever. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | A pass that leaves vacated cells but records no fill source returns error `-45436`. Pinned by `Execute Error - no fill progress (-45436)`. | -| `RemoveFlaggedFeaturesFilter-D4` | A FeatureId that was negative or not less than the feature tuple count indexed the flag vector out of bounds during marking. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | Every FeatureId is validated before any modification; a bad value returns error `-45435` naming the cell, value and valid range. Pinned by `Execute Error - FeatureId out of range (-45435)`. | +| `RemoveFlaggedFeaturesFilter-D3` | With fill on, when every cell belonged to a flagged feature and the only unflagged features owned no cells, no vacated cell had a fill source and the loop spun forever. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | A pass that leaves vacated cells but fills none of them returns error `-45436`. The Feature Ids array is always copied even when listed in *Attribute Arrays to Ignore* (warning `-45438`), which closed a second route to the same hang. Pinned by `Execute Error - no fill progress (-45436)` and `Fill cannot ignore the Feature Ids array (-45438 warning)`. | +| `RemoveFlaggedFeaturesFilter-D4` | A FeatureId that was negative or not less than the feature tuple count indexed the flag vector out of bounds during marking. | DREAM.3D 6.5.171; DREAM3D-NX v7.0.0 through v7.4.2. | Every FeatureId is validated before any modification; a bad value returns error `-45435` naming the cell, value and valid range, and a tuple count that differs from the geometry cell count returns `-45437`. Pinned by `Execute Error - FeatureId out of range (-45435)` and `Execute Error - Feature Ids tuple count differs from the geometry (-45437)`. | | `RemoveFlaggedFeaturesFilter-D5` | The extract path tested the preflight result where it should have tested the execute result of its sub-filters, so an execute failure was ignored, and a preflight failure threw `std::runtime_error` instead of returning an error. | DREAM3D-NX v7.0.0 through v7.4.2. Not applicable to 6.5.171. | Sub-filter preflight and execute results are checked and returned as errors `-53901` through `-53904` with the sub-filter's message. Covered by inspection; see Code path coverage rows 6, 7, 11, 12. | -| `RemoveFlaggedFeaturesFilter-D6` | A flagged feature that owns no cell has an empty bounding box. SIMPLNX passed it to the crop, which failed preflight and threw. DREAM.3D 6.5.171 crops a 1x1x1 geometry at origin (-1, -1, -1) containing cell 0. | DREAM.3D 6.5.171 (bogus geometry); DREAM3D-NX v7.0.0 through v7.4.2 (uncaught exception). | The feature is skipped with warning `-53905` and no geometry is created. Pinned by `Extract - flagged feature with no cells is skipped with a warning`. | +| `RemoveFlaggedFeaturesFilter-D6` | A flagged feature that owns no cell has an empty bounding box. SIMPLNX passed it to the crop, which failed preflight and threw. DREAM.3D 6.5.171 crops a 1x1x1 geometry at origin (-1, -1, -1) containing cell 0. | DREAM.3D 6.5.171 (bogus geometry); DREAM3D-NX v7.0.0 through v7.4.2 (uncaught exception). | The features are skipped and one warning `-53905` lists them; no geometry is created for them. Pinned by `Extract - flagged feature with no cells is skipped with a warning`. | A preflight cleanup that is not a deviation: the filter emitted its own NeighborList warning (`-11505`) and the shared helper emitted a second one (`-5558`) for the same arrays. The duplicate was removed; the test asserts exactly one warning. +## Review findings + +Five review passes were run on the change set only (adversarial, nit-picky senior engineer, CPU performance, memory, out-of-core). Findings that changed the code: + +- Adversarial: listing the Feature Ids array in *Attribute Arrays to Ignore* with fill on still hung, because the guard tested whether a source was chosen, not whether a cell was filled. The guard now counts filled cells and the Feature Ids array is never ignored (warning `-45438`). The Feature Ids tuple count is now checked against the geometry (`-45437`). +- Memory: every extracted geometry received a copy of the internal `tempBounds` array, and the array leaked on every error return. The bounds are copied to a local buffer and the array is deleted before the first crop. The dead `CreateArrayAction` in preflight (sized to the cell count) was removed. +- CPU: the per-vacated-cell `std::vector` allocations in the neighbor tally were replaced by fixed `std::array`s; the cell-array list is built once instead of once per pass; the validation pass polls the cancel flag. The `>= 0` fix also removes a per-pass allocation storm on background cells. +- Senior engineer: `-45436` reports the number of stuck cells; the empty-feature warning is aggregated; sub-filter error messages carry the bounds and a count of further errors; Doxygen completed on the new functions; inputs of the scan are `const`. +- Out-of-core: no access pattern of an existing loop changed; the validation pass is one extra chunk-sequential read. The `simplnx-ooc-Rel` caveat above was added. + ## Code path coverage -20 of 28 paths are assertion-covered. The uncovered paths are cancellation and defensive failure paths of the sub-filters and the compaction helper. +23 of 31 paths are assertion-covered. The uncovered paths are cancellation and defensive failure paths of the sub-filters and the compaction helper. -Source: `src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp` (222 lines) and `src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp` (333 lines), plus preflight in `Filters/RemoveFlaggedFeaturesFilter.cpp` (212 lines). +Source: `src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RemoveFlaggedFeatures.cpp` and `src/Plugins/SimplnxCore/src/SimplnxCore/utils/FeatureRemovalUtilities.cpp`, plus preflight in `Filters/RemoveFlaggedFeaturesFilter.cpp`. Phases: (a) preflight, (b) extract (bounding boxes, then one crop per flagged feature), (c) removal marking, (d) iterative fill, (e) compaction. | # | Phase | Path | Test case | |---|---|---|---| | 1 | (a) Preflight | Parent of the flag array is not an Attribute Matrix -> error `-9892` | `Preflight Error - flag array parent is not an Attribute Matrix (-9892)` | -| 2 | (a) Preflight | *Extract* or *Extract then Remove* -> temporary `tempBounds` array created and deleted after execution | `Class 1 Oracle - Extract crops the bounding box of each flagged feature` (asserts `tempBounds` is gone) | +| 2 | (b) Extract | Temporary `tempBounds` array is created by the `ComputeFeatureRect` sub-filter and deleted by the algorithm before the first crop, so no extracted geometry carries it | `Class 1 Oracle - Extract crops the bounding box of each flagged feature` (asserts `tempBounds` is absent from the source and from the extracted geometry) | | 3 | (a) Preflight | *Remove* or *Extract then Remove* -> cell and feature groups marked modified, NeighborLists scheduled for deletion, one warning `-5558` | `Preflight Warning - NeighborLists are removed (-5558)` (`Functionality = 0`, `Functionality = 2`) | | 4 | (a) Preflight | *Extract* -> no NeighborList warning, NeighborLists kept | `Preflight Warning - NeighborLists are removed (-5558)` (`Extract alone does not remove NeighborLists`) | | 5 | (b) Extract | Flag array is not Bool or UInt8 -> error `-53900` | *Not directly tested. The parameter type check in preflight prevents it; defensive.* | | 6 | (b) Extract | `ComputeFeatureRect` preflight fails -> error `-53901` | *Not directly tested. Requires an internal inconsistency the filter's own preflight already rejects; defensive.* | -| 7 | (b) Extract | `ComputeFeatureRect` execute fails -> error `-53902` | *Not directly tested. The sub-filter fails only on a FeatureId beyond the feature tuple count, which path 14 rejects first in the removal modes; defensive in Extract-only mode.* | -| 8 | (b) Extract | Flagged feature owns no cell -> warning `-53905`, no geometry | `Extract - flagged feature with no cells is skipped with a warning` | +| 7 | (b) Extract | `ComputeFeatureRect` execute fails -> error `-53902`, `tempBounds` removed | *Not directly tested. The sub-filter fails on a FeatureId beyond the feature tuple count. Extract runs before removal, so in *Extract* and *Extract then Remove* this path is reached before path 14; defensive.* | +| 8 | (b) Extract | One or more flagged features own no cell -> one warning `-53905` listing them, no geometry for them | `Extract - flagged feature with no cells is skipped with a warning` (two empty features, one warning) | | 9 | (b) Extract | Flagged feature -> new geometry `-` with the bounding-box dimensions, origin, all cell arrays and the feature Attribute Matrix | `Class 1 Oracle - Extract crops the bounding box of each flagged feature`, `Class 1 Oracle - Extract zero-pads names and handles several features`, `Class 1 Oracle - Extract then Remove with fill` | | 10 | (b) Extract | Unflagged feature -> no geometry | `Class 1 Oracle - Extract zero-pads names and handles several features` | | 11 | (b) Extract | Crop preflight fails -> error `-53903` | *Not directly tested. Reachable before this branch through path 8; now unreachable for valid bounds; defensive.* | | 12 | (b) Extract | Crop execute fails -> error `-53904` | *Not directly tested. Defensive.* | | 13 | (b) Extract | Cancel between crops -> return | *Not directly tested. Requires cancel-signal injection.* | | 14 | (c) Marking | A FeatureId is negative or >= feature tuple count -> error `-45435`, nothing modified | `Execute Error - FeatureId out of range (-45435)` (both sections) | +| 14a | (c) Marking | Feature Ids tuple count differs from the geometry cell count -> error `-45437`, nothing modified | `Execute Error - Feature Ids tuple count differs from the geometry (-45437)` | +| 14b | (c) Marking | Fewer than two feature tuples -> error `-45433`, nothing modified | *Not directly tested. A one-tuple feature Attribute Matrix cannot hold a flagged feature; defensive.* | | 15 | (c) Marking | Every feature flagged -> error `-45433`, nothing modified | `Execute Error - all features flagged (-45433)` (`Fill = false`, `Fill = true`) | | 16 | (c) Marking | Fill off: flagged cell -> 0 | `Class 1 Oracle - Remove without fill` | | 17 | (c) Marking | Fill on: flagged cell -> -1 | Every `Class 1 Oracle - Fill *` test | | 18 | (d) Fill | Vacated cell with a single valid neighbor, or all-distinct neighbors: first sighting records the source | `Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` (cells 3 and 9) | | 19 | (d) Fill | Majority vote: the source moves only when a feature's tally strictly exceeds the previous maximum | `Class 1 Oracle - Fill majority vote and second iteration (3x3x3)` (cell 13, 4 to 2) | -| 20 | (d) Fill | 1-vs-1 tie -> first-seen feature in -Z, -Y, -X, +X, +Y, +Z order | `Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` (cell 4), `(3x3x3)` (cell 0) | +| 20 | (d) Fill | 1-vs-1 tie -> first-seen feature in -Z, -Y, -X, +X, +Y, +Z order | `Class 1 Oracle - Fill from distinct first-sighting neighbors (issue #1698)` (cells 3 and 4) | | 21 | (d) Fill | Vacated cell with no non-negative neighbor this pass -> filled in a later pass from a neighbor that was itself filled | `(issue #1698)` (cell 4 via cell 3), `(3x3x3)` (cell 0 via cell 9 via cell 18) | | 22 | (d) Fill | Background cell (FeatureId 0) is skipped as a target and counted as a source | `Class 1 Oracle - Fill treats FeatureId 0 as a source and never a target` (cell 14 -> 0) | -| 23 | (d) Fill | Vacated cells remain and none has a source -> error `-45436`, cells left at -1 | `Execute Error - no fill progress (-45436)` | +| 23 | (d) Fill | Vacated cells remain and the pass filled none -> error `-45436`, cells left at -1 | `Execute Error - no fill progress (-45436)` | +| 23a | (d) Fill | Feature Ids array listed in the ignore list -> removed from the list, warning `-45438`, fill proceeds | `Fill cannot ignore the Feature Ids array (-45438 warning)` | | 24 | (d) Fill | Every non-ignored cell array is copied from the source cell; ignored arrays are untouched | `Class 1 Oracle - Fill copies every non-ignored Cell array` and the `IgnoredValue` checks in every fill test | | 25 | (d) Fill | Face neighbors outside the volume are excluded | 5x2x1 and 4x4x1 fixtures (Y and Z faces), 3x3x3 fixture (all six faces) | | 26 | (d) Fill | Cancel during a pass -> return | *Not directly tested. Requires cancel-signal injection.* | @@ -133,11 +146,13 @@ Phases: (a) preflight, (b) extract (bounding boxes, then one crop per flagged fe | `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Fill majority vote and second iteration (3x3x3)` | new-for-V&V | Fixture E; asserts all 27 `FeatureIds`, the five copy sources including the two-pass chain into cell 0, ignored array and compaction. 19 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - all features flagged (-45433)` | new-for-V&V | `DYNAMIC_SECTION` for fill off and on; asserts the exact code and that no array was modified. 21 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - FeatureId out of range (-45435)` | new-for-V&V | Sections for a value equal to the tuple count and for a negative value; asserts the code and that `FeatureIds` is unmodified. Covers D4. 15 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - Feature Ids tuple count differs from the geometry (-45437)` | new-for-V&V | A 4-tuple Feature Ids array outside the 10-cell geometry; asserts the code and that the array is unmodified. Covers D4. 9 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Execute Error - no fill progress (-45436)` | new-for-V&V | 4x1x1 where the only unflagged feature owns no cell; asserts the code and that all cells are left at -1. Covers D3. 8 assertions. | -| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract crops the bounding box of each flagged feature` | new-for-V&V | Fixture B extract; asserts the original is untouched, the new geometry's dimensions (2,1,1), origin (1,3,0), spacing, all three cell arrays, the carried feature Attribute Matrix, and deletion of `tempBounds`. 35 assertions. | -| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract zero-pads names and handles several features` | new-for-V&V | 12-feature 1D fixture; asserts two-digit padding (`-03`, `-11`), per-feature dimensions, origin and cell values, and that unflagged features produce no geometry. 39 assertions. | -| `SimplnxCore::RemoveFlaggedFeaturesFilter: Extract - flagged feature with no cells is skipped with a warning` | new-for-V&V | Asserts warning `-53905`, that feature 3 is still extracted and feature 4 produces no geometry. Covers D6. 24 assertions. | -| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract then Remove with fill` | new-for-V&V | Fixture B; asserts the extracted geometry (from the unmodified input) and the filled original in one run. 31 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Fill cannot ignore the Feature Ids array (-45438 warning)` | new-for-V&V | Fixture A with the Feature Ids array in the ignore list; asserts exactly one warning `-45438` and the same output as the issue #1698 test. Hung before this branch. Covers D3. 18 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract crops the bounding box of each flagged feature` | new-for-V&V | Fixture B extract; asserts the original is untouched, the new geometry's dimensions (2,1,1), origin (1,3,0), spacing, all three cell arrays, the carried feature Attribute Matrix, and that `tempBounds` is absent from both the source and the extracted geometry. 35 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract zero-pads names and handles several features` | new-for-V&V | 12-feature 1D fixture; asserts two-digit padding (`-03`, `-11`), per-feature dimensions, origin and cell values, and that unflagged features produce no geometry. 37 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Extract - flagged feature with no cells is skipped with a warning` | new-for-V&V | Two flagged features own no cell; asserts exactly one warning `-53905` that names both, that feature 3 is still extracted and that features 4 and 5 produce no geometry. Covers D6. 27 assertions. | +| `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 1 Oracle - Extract then Remove with fill` | new-for-V&V | Fixture B; asserts the extracted geometry (from the unmodified input) and the filled original in one run. 30 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Error - flag array parent is not an Attribute Matrix (-9892)` | new-for-V&V | Flag array placed in a plain `DataGroup`; asserts invalid preflight and the code. 5 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Preflight Warning - NeighborLists are removed (-5558)` | new-for-V&V | `DYNAMIC_SECTION` for *Remove* and *Extract then Remove* asserts exactly one warning with code `-5558` and that the NeighborList is gone after execution; a third section asserts *Extract* emits no warning and keeps it. 23 assertions. | | `SimplnxCore::RemoveFlaggedFeaturesFilter: Class 4 Invariants - Small IN100 remove small features with fill` | new-for-V&V | Loads `6_5_test_data_1_v2`, flags the 275 features with fewer than 100 cells, fills, and checks the six invariants listed in the Oracle section over 1,000,000 cells with aggregated counts. 751 assertions. | @@ -146,7 +161,7 @@ Phases: (a) preflight, (b) extract (bounding boxes, then one crop per flagged fe | `SimplnxCore::RemoveFlaggedFeatures: Test Extract Algorithm` | retired | Checked only two `FeatureIds` values in the extracted geometry. Replaced by `Class 1 Oracle - Extract crops the bounding box of each flagged feature`, which also checks dimensions, origin, spacing, every cell array and the feature Attribute Matrix. | | `SimplnxCore::RemoveFlaggedFeatures: Test Extract then Remove Algorithm` | retired | Fill off; same two-value extract check. Replaced by `Class 1 Oracle - Extract then Remove with fill`. | -There are 16 active `TEST_CASE`s: 1 kept (modified) and 15 new for this V&V cycle; 3 retired. +There are 18 active `TEST_CASE`s: 1 kept (modified) and 17 new for this V&V cycle; 3 retired. `KeepRemoveRankedFeaturesTest.cpp` gained one section (`Fill terminates when the input has background cells`) because that filter shares the fill utility and inherited D2. ## Exemplar archive @@ -172,8 +187,8 @@ See `vv/deviations/RemoveFlaggedFeaturesFilter.md` for the root cause, affected |-----------|---------------------| | `RemoveFlaggedFeaturesFilter-D1` | SIMPLNX v7.0.0 through v7.4.1 never returned when a vacated cell's valid neighbors were all distinct features or a single neighbor; 6.5.171 completed. Fixed in v7.4.2. | | `RemoveFlaggedFeaturesFilter-D2` | SIMPLNX through v7.4.2 never returned when the input contained FeatureId 0 cells and fill was on; 6.5.171 completed. Fixed on this branch. | -| `RemoveFlaggedFeaturesFilter-D3` | SIMPLNX returns `-45436` when no vacated cell has a fill source; 6.5.171 loops forever. | -| `RemoveFlaggedFeaturesFilter-D4` | SIMPLNX returns `-45435` for a FeatureId outside `[0, tuple count)`; 6.5.171 reads out of bounds and silently zeroed the cell in the A/B run. | +| `RemoveFlaggedFeaturesFilter-D3` | SIMPLNX returns `-45436` when a fill pass fills no cell, and always copies the Feature Ids array even when it is listed as ignored (warning `-45438`); 6.5.171 loops forever in both situations. | +| `RemoveFlaggedFeaturesFilter-D4` | SIMPLNX returns `-45435` for a FeatureId outside `[0, tuple count)` and `-45437` for a tuple count that differs from the geometry; 6.5.171 reads out of bounds and silently zeroed the cell in the A/B run. | | `RemoveFlaggedFeaturesFilter-D5` | SIMPLNX returns `-53901` through `-53904` when an extract sub-filter fails; earlier SIMPLNX ignored execute failures and threw on preflight failures. Not applicable to 6.5.171. | | `RemoveFlaggedFeaturesFilter-D6` | For a flagged feature with no cells SIMPLNX warns `-53905` and creates nothing; 6.5.171 creates a 1x1x1 geometry at origin (-1, -1, -1) holding cell 0. | | `RemoveFlaggedFeaturesFilter-D7` | Extracted geometries are named `-` and carry the feature Attribute Matrix; 6.5.171 names them `Feature_` and copies only cell data. | diff --git a/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md index 62154d3f56..34f1aeb253 100644 --- a/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/vv/deviations/RemoveFlaggedFeaturesFilter.md @@ -54,9 +54,9 @@ On valid input (every FeatureId in range, at least one unflagged feature that ow **Symptom:** With fill enabled, when every cell belongs to a flagged feature and the unflagged features own no cells, SIMPLNX returns error `-45436` and leaves the cells at -1; DREAM3D 6.5.171 loops forever. Reproduced on a 4x1x1 volume with `FeatureIds = [1, 1, 1, 1]`, three feature tuples, and only feature 1 flagged: 6.5.171 was still running when killed after 30 seconds. -**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). The all-flagged guard only inspects the flag array, so an unflagged feature that owns no cells satisfies it. After marking, no cell is non-negative, so no vacated cell ever finds a fill source and neither implementation detected the lack of progress. SIMPLNX now tracks whether a pass recorded any fill source and stops with a deterministic error when vacated cells remain without one. This is the same class of defect as `RequireMinNumNeighborsFilter-D3`. +**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). The all-flagged guard only inspects the flag array, so an unflagged feature that owns no cells satisfies it. After marking, no cell is non-negative, so no vacated cell ever finds a fill source and neither implementation detected the lack of progress. SIMPLNX now counts the cells that each pass actually fills and stops with a deterministic error when vacated cells remain and the count is zero. A second route to the same non-termination, listing the Feature Ids array itself in *Attribute Arrays to Ignore* so that sources were chosen but the Feature Ids array was never overwritten, is closed by always copying that array and warning (`-45438`). 6.5.171 also loops forever in that case (`voxelArrayNames.removeAll` honors the request). This is the same class of defect as `RequireMinNumNeighborsFilter-D3`. -**Affected users:** Users whose feature Attribute Matrix contains features that own no cells, when the flag array removes every feature that does own cells. Valid inputs are unaffected. +**Affected users:** Users whose feature Attribute Matrix contains features that own no cells, when the flag array removes every feature that does own cells; and users who list the Feature Ids array in *Attribute Arrays to Ignore* with fill enabled. Other inputs are unaffected. **Recommendation:** `trust SIMPLNX`. The error names the array and explains the two ways to correct the input; the legacy behavior is an unbounded execution. @@ -70,9 +70,9 @@ On valid input (every FeatureId in range, at least one unflagged feature that ow | **Filter UUID** | `6e8cc6ec-8b9b-402e-9deb-85bd1cdba743` | | **Status** | active | -**Symptom:** When a FeatureId is negative or not less than the feature tuple count, SIMPLNX returns error `-45435` before modifying anything; DREAM3D 6.5.171 indexes its flag vector out of bounds. In the A/B run (5x2x1, cell 6 set to 5 with five feature tuples) 6.5.171 completed and silently zeroed that cell, which depends on whatever memory followed the vector. +**Symptom:** When a FeatureId is negative or not less than the feature tuple count, SIMPLNX returns error `-45435` before it modifies anything; DREAM3D 6.5.171 indexed its flag vector out of bounds. In the A/B run (5x2x1, cell 6 set to 5 with five feature tuples) 6.5.171 completed and silently zeroed that cell. The result depended on whatever memory followed the vector. -**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). `remove_flaggedfeatures()` evaluates `activeObjects[gnum]` for every cell without validating `gnum`. SIMPLNX now validates every FeatureId against the flag count in a read-only pass before the marking pass. This is the same class of defect as `RequireMinNumNeighborsFilter-D2`. +**Root cause:** `bug` in DREAM3D 6.5.171 (and in SIMPLNX before this branch). `remove_flaggedfeatures()` evaluates `activeObjects[gnum]` for every cell without validating `gnum`. SIMPLNX now validates every FeatureId against the flag count in a read-only pass before the marking pass, and also returns `-45437` when the Feature Ids tuple count differs from the geometry cell count, which neither implementation checked before. This is the same class of defect as `RequireMinNumNeighborsFilter-D2`. **Affected users:** Users with a Feature Ids array that does not correspond to the selected feature Attribute Matrix, for example after selecting the wrong Attribute Matrix or after a partial renumbering. Valid inputs are unaffected. From 6ca8587df9dff6032feb5141dc15630e9780af12 Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Thu, 3 Sep 2026 11:09:21 -0400 Subject: [PATCH 3/3] DOC: Name the OneDrive archive folder in the Remove Flagged Features V&V report * Point the legacy-comparison section at the actual archive location (V&V OneDrive library, folder RemoveFlaggedFeaturesFilter) instead of a placeholder name. Signed-off-by: Michael Jackson --- src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md index c6359f54ab..addc38a613 100644 --- a/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md +++ b/src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md @@ -179,7 +179,7 @@ The Class 4 test uses this shared input archive: ## Deviations from DREAM3D 6.5.171 -On 2026-09-02, DREAM3D 6.5.171 and SIMPLNX were run on legacy-native copies of fixtures A, B and E with fill on and off (five removal cases) and on fixture B with *Extract*. The inputs were authored with the shared h5py legacy writer so both runners read identical files. All 25 removal array comparisons (`FeatureIds`, `CellValue`, `IgnoredValue`, `Int32DataSet`, `Active`) matched the analytical oracle and each other exactly. The extracted geometry's dimensions, origin and three cell arrays matched. Three further fixtures were run to characterize the error-path deviations: the no-progress twin (D3), an out-of-range FeatureId (D4) and a flagged feature with no cells (D6). The pipelines, inputs, outputs, logs and comparison script are archived outside the repository (`FilterVerification_RemoveFlaggedFeaturesFilter`) per the archival policy. +On 2026-09-02, DREAM3D 6.5.171 and SIMPLNX were run on legacy-native copies of fixtures A, B and E with fill on and off (five removal cases) and on fixture B with *Extract*. The inputs were authored with the shared h5py legacy writer so both runners read identical files. All 25 removal array comparisons (`FeatureIds`, `CellValue`, `IgnoredValue`, `Int32DataSet`, `Active`) matched the analytical oracle and each other exactly. The extracted geometry's dimensions, origin and three cell arrays matched. Three further fixtures were run to characterize the error-path deviations: the no-progress twin (D3), an out-of-range FeatureId (D4) and a flagged feature with no cells (D6). The pipelines, inputs, outputs, logs and comparison script are archived outside the repository in the V&V OneDrive library, folder `RemoveFlaggedFeaturesFilter` (59 files, uploaded 2026-09-03), per the archival policy. See `vv/deviations/RemoveFlaggedFeaturesFilter.md` for the root cause, affected users and recommendation for each deviation.