[imikejackson]

Summary

These filters take Cell Feature Ids and an Image Geometry (QuickSurfaceMesh also accepts a Rectilinear Grid). They modify Feature Ids, the other arrays in the same Attribute Matrix, or the Feature Attribute Matrix. Their algorithms are spatial: they use the geometry's dimensions or cell neighbors. Before this change, any Feature Ids DataArray was accepted, and each filter found the arrays to modify through featureIds.getParent(). If Feature Ids lived in a different Attribute Matrix, the filter modified the wrong arrays and left the geometry's Cell Attribute Matrix out of sync. Preflight gave no error.

New rule for these filters:

1. Cell Feature Ids must be a child of the selected geometry's Cell Attribute Matrix (the matrix the geometry points to, not any matrix with the same number of tuples).
2. Preflight and execute take the arrays to modify from the geometry's Cell Attribute Matrix.
3. Every other Cell array that the filter modifies, and every entry in an "arrays to ignore" list, must be in that same Attribute Matrix. An ignore list is checked only when the filter uses it (for example, only when Fill is on).
4. Removal filters: the Feature Attribute Matrix (the parent of the ranking, flagged, Number of Neighbors or Num. Cells array) must be an Attribute Matrix directly under the geometry, and must not be its Cell Attribute Matrix.

Arrays that a filter only reads keep the existing rule: their number of tuples must match, and any Attribute Matrix is allowed.

The checks use new helpers in DataArrayUtilities (ValidateArrayInGeometryCellData, ValidateArraysInGeometryCellData, ValidateFeatureAttributeMatrixInGeometry). The helpers compare objects, not path strings. An Attribute Matrix that can be reached by more than one DataPath (for example, a geometry inside a montage) is therefore accepted. The filters then rewrite the selected paths to the geometry's Cell Attribute Matrix path, so an ignored array that was selected through the other path is still ignored.

Each filter has its own commit, regression tests that fail on develop, and a short paragraph in its documentation. No test data archive is added or changed.

Behavior changes (new preflight errors)

Filter	Feature Ids	Other Cell arrays	Feature Attribute Matrix
ErodeDilateBadData	−14605	Attribute Arrays to Ignore −14604	—
ErodeDilateCoordinationNumber	−16801	Attribute Arrays to Ignore −16802	—
FillBadData	−16501	Cell Phases (when Store Defects as New Phase is on) −16502; Attribute Arrays to Ignore −16503	—
KeepRemoveRankedFeatures	−78125	Attribute Arrays to Ignore (Fill on) −78126	−78127 (replaces −78100)
RemoveFlaggedFeatures	−9893 (all operations)	Attribute Arrays to Ignore (removing with Fill on) −9894	−9895 (replaces −9892)
RequireMinNumNeighbors	−55575; the existing tuple-count check −55571 is kept, because Cell Attribute Matrix membership does not guarantee one tuple per cell	Cell Arrays to Ignore −55573	−55574
RequireMinimumSizeFeatures	−5562	—	−5563 (replaces −5557)
QuickSurfaceMesh	Both modes: number of tuples must equal the geometry's number of cells, −76533 (no check before). Fix Problem Voxels on: also −76532, and preflight now reports Feature Ids as modified.	—	—

FillBadData also had an inconsistency: preflight reported the arrays under Feature Ids' parent as modified, while execute modified the geometry's Cell Attribute Matrix. Both now use the geometry's Cell Attribute Matrix.

Review fixes (commits after 01f0d25)

• RequireMinNumNeighbors keeps the -55571 tuple-count check after the new membership check. The algorithm iterates the geometry's cell count over the Feature Ids store, so the check is still needed.
• QuickSurfaceMesh runs the -76533 tuple-count check in both modes, then the -76532 membership check only when fixing problem voxels.
• ValidateFeatureAttributeMatrixInGeometry now decides "directly under the geometry" with geometry->contains(attributeMatrix) instead of resolving the selected path's parent, so it compares objects like the two Cell helpers.
• The two ErodeDilate algorithms no longer rebuild the Feature Ids path; executeImpl is the single normalization point.
• Test include blocks restored; ErodeDilateBadData and FillBadData V&V reports list the new preflight paths and tests.

Not in this PR

Crop Geometry (Image) and Resample Data (Image Geometry) with Renumber Features need the same Feature Ids rule. Execute builds the renumbered Feature Ids path from the Cell Attribute Matrix name, so a Feature Ids array that lives elsewhere causes a null dereference in SamplingUtils.hpp. Those two filters are being reworked on topic/correct_feature_am_updates, so their check will go there, to avoid conflicts.

Note for reviewers

topic/correct_feature_am_updates also edits the end of preflightImpl in the four removal filters, plus ErodeDilateBadDataTest, FillBadDataTest, RemoveFlaggedFeaturesTest, two docs and vv/RemoveFlaggedFeaturesFilter.md. New code in this PR is placed away from those lines where possible. The remaining conflicts should be small.

Test plan

• Each new regression test passes. Each one fails on develop because preflight accepts the misplaced array.
• RemoveFlaggedFeatures and FillBadData tests pass for both algorithm scenarios.
• The full test suite passes (3223 of 3224 locally on macOS; the test not run needs Python, which the local build does not include).
• The 15 uses of these filters in the shipped pipelines (simplnx, DREAM3D-NX, plugins) already follow the rule. Preflighting every simplnx example pipeline shows no new errors.
• CI passes on all supported platforms.

[imikejackson]

Not a review.

[imikejackson]

@joeykleingers All 4 comments are addressed in 4488feb. Each thread has a reply and is resolved. The branch is also rebased onto the current develop (#1817). The only conflict was the include block in test/DataArrayTest.cpp. Please review again.

[joeykleingers]

Draft from the automated pr-review run at c4b3b2b466. Edit freely before submitting.

Findings not attached to a changed line

• [Low] src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md:164 RemoveFlaggedFeatures V&V report still has no rows for -9893/-9894 or the two new tests, and one test description is wrong

src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md:164 RemoveFlaggedFeatures V&V report still has no rows for -9893/-9894 or the two new tests, and one test description is wrong

[Low] RemoveFlaggedFeatures V&V report still has no rows for -9893/-9894 or the two new tests, and one test description is wrong

This PR adds two preflight errors to the filter:

• -9893: Feature Ids is outside the Cell Attribute Matrix (all operations).
• -9894: the ignore list is outside the Cell Attribute Matrix (Remove or Extract-then-Remove with Fill on).

Neither code has a row in the path table; only row 1 (line 106) was renamed to -9895. The inventory leaves out RemoveFlaggedFeaturesFilter: Strict Feature Ids and RemoveFlaggedFeaturesFilter: Shared Cell Paths and Selected Feature Matrix, so the "18 active TEST_CASEs" count at line 164 is stale.

Inventory row 149 still describes the -45437 test as "A 4-tuple Feature Ids array outside the 10-cell geometry". Preflight now rejects that setup with -9893, and the test now grows the geometry's X dimension instead.

Fix:

• Add path rows for -9893 and -9894, following ErodeDilateBadData rows 12–14 and FillBadData rows 18–21.
• Add the two inventory rows and recount line 164.
• Change row 149 to: "Geometry X dimension is increased by one so that Feature Ids no longer has one tuple per cell; asserts the code and that the array is unmodified."

---

PR 1829 Code Review

Source: review of commit c4b3b2b (BlueQuartzSoftware/simplnx topic/strict_featureids_cell_data → develop @ 01809c2), author imikejackson. Coverage: all 31 changed files. That is 8 filter docs, 8 filters, 8 plugin tests, 4 V&V reports, DataArrayUtilities.{hpp,cpp} and test/DataArrayTest.cpp. This is a re-review after Joey's review was requested again. The previous automated review covered 4488feb.

Since the last review

What changed since 4488feb

• The base did not move. It is still 01809c2, and the same 31 files are changed.
• Direct tests for MapToGeometryCellDataPath. DataArrayUtilities::Geometry attribute matrix validation (test/DataArrayTest.cpp:193–198) now checks both overloads:
  • a path from an unrelated parent and a path from a sibling Attribute Matrix both map to the geometry's Cell path;
  • a path that is already correct is returned unchanged;
  • a two-element vector keeps its order;
  • an empty vector returns an empty vector.
• No other change found. The filter code, the docs and the other V&V reports in the head diff match what the previous report described.

Earlier findings

#	Earlier finding	Status
1	[Low] RemoveFlaggedFeatures V&V has no -9893/-9894 rows or new tests, has a stale count, and row 149 is now wrong	Remains. At head, the path table still has only the -9895 rename (row 1, line 106). Inventory line 149 still says "A 4-tuple Feature Ids array outside the 10-cell geometry". Line 164 still says "18 active TEST_CASEs".
2	[Nit] MapToGeometryCellDataPath has no direct unit test	Resolved. The five CHECKs at test/DataArrayTest.cpp:193–198 cover every case the earlier finding suggested.

Findings

• [Low] RemoveFlaggedFeatures V&V report still has no rows for the new -9893/-9894 paths or the two new tests, and one test description is wrong
  File: src/Plugins/SimplnxCore/vv/RemoveFlaggedFeaturesFilter.md:164 (also :106, :149)
  This PR adds these preflight errors to the filter (RemoveFlaggedFeaturesFilter.cpp:492, :501):

  • -9893: Feature Ids is outside the Cell Attribute Matrix (all operations).
  • -9894: the ignore list is outside the Cell Attribute Matrix (Remove or Extract-then-Remove with Fill on).

  Neither code has a row in the path table. The inventory leaves out two new tests, so the "18 active TEST_CASEs" count at line 164 is stale:

  • RemoveFlaggedFeaturesFilter: Strict Feature Ids (RemoveFlaggedFeaturesTest.cpp:1390);
  • RemoveFlaggedFeaturesFilter: Shared Cell Paths and Selected Feature Matrix (:1496).

  This PR also rewrote the -45437 test (RemoveFlaggedFeaturesTest.cpp:662–680). The test now makes the mismatch by growing the geometry's X dimension. Inventory row 149 still says "A 4-tuple Feature Ids array outside the 10-cell geometry", but preflight now rejects that setup with -9893. A V&V reader would expect a setup that this PR forbids.

  Fix:

  • Add path rows for -9893 and -9894, following ErodeDilateBadData rows 12–14 and FillBadData rows 18–21.
  • Optionally extend row 1 to say the Strict Feature Ids sections also cover -9895.
  • Add the two inventory rows and recount line 164.
  • Change row 149 to: "Geometry X dimension is increased by one so that Feature Ids no longer has one tuple per cell; asserts the code and that the array is unmodified."

• [Nit] A bool flag array gets an integer literal next to false/true
  File: src/Plugins/SimplnxCore/test/RemoveFlaggedFeaturesTest.cpp:1549 (Shared Cell Paths and Selected Feature Matrix)
  featureValues is created with CreateTestDataArray<bool>. The next two lines assign false and true, but this line assigns 0. The code works, but the mixed literals read like a copy from the int32 version of the test in the other filters. Fix: (*featureValues)[0] = false;.

Correctness / Robustness

No issues found. Checked:

• Validation runs before the helper is used. All 8 filters call MapToGeometryCellDataPath (or getCellDataPath()) only after the membership validators. Those validators return an error when the geometry has no Cell Attribute Matrix, so getDataRefAs<ImageGeom>(…).getCellDataPath() cannot throw in preflight. Execute runs only after a valid preflight.
• Unvalidated ignore lists are harmless. KeepRemoveRanked with Fill off, and RemoveFlagged in Extract mode or with Fill off, skip ignore-list validation. Execute still maps those lists by target name, so a misplaced ignored array could map onto an unrelated Cell array with the same name. This is harmless because the algorithms read IgnoredDataArrayPaths only when Fill is on and the operation is not Extract.
• QuickSurfaceMesh order. The -76533 tuple-count check runs in both modes, before the -76532 membership check. The modification notice reports the geometry's Cell Attribute Matrix path even when Feature Ids is selected through an alias, and the alias test section checks this.
• RequireMinNumNeighbors order. The checks run in this order: -55575, -55571, -55573, -55574. This matches the user doc's failure list.
• Null checks. KeepRemoveRanked and RemoveFlagged still rely on ValidateFeatureAttributeMatrixInGeometry to guarantee that featureAmPtr and cellFeatureAmPtr are non-null. This still holds.

C++ checks

No other issues found. No new heap ownership. The helpers take const& and return by value. No threading changes. <string_view> is included for the new parameters. Errors use the existing Result<>/MakeErrorResult pattern.

Documentation

No issues found in the Doxygen or the user docs. Both MapToGeometryCellDataPath overloads and the three validators have @brief, every @param and a @return, and match their signatures. The 8 filter docs match the code, including the conditional rules (Fill on, Store as New Phase, Fix Problem Voxels). The only V&V gap is the RemoveFlaggedFeatures report covered above.

Formatting

clang-format.txt: "Every changed C/C++ file is formatted." No findings.

Tests

Every new preflight error has a test that checks the exact code and the paths in the message. The tests also cover:

• the conditional checks: Fill off, Extract only, Store as New Phase off;
• selection through an alias path, including execute-time checks that ignored arrays stay unchanged and that only the selected Feature Attribute Matrix is compacted;
• read-only arrays in another Attribute Matrix, which still pass.

DataArrayTest.cpp now covers both mapping overloads and all three validators, on both Image and Rectilinear Grid geometries, including the case where the geometry has no Cell Attribute Matrix.

Missing or failed evidence

• I did not build or run any tests (read-only review, no shell). All test-plan checkboxes in the PR are unchecked. The author reports 3223/3224 tests passing locally on macOS. CI status was not in the evidence bundle.
• I could not check the claim that the 15 shipped pipeline uses already follow the rule. Pipelines from other repositories were not available.
• I had no 4488feb..c4b3b2b interdiff. I worked out "since the last review" by comparing the previous report with the full head diff. A small change that the previous report did not describe could be missed.

Confirmed correct

• Membership compares objects, not path strings. IsChildOfAttributeMatrix compares parent ids. ValidateFeatureAttributeMatrixInGeometry uses geometry->contains(...) plus an id check against Cell Data. Both accept an Attribute Matrix reached through a second parent, and DataArrayTest.cpp:2720–2721 (diff numbering) proves this.
• One mapping point. Every rewritten path comes from the geometry's getCellDataPath(), the same path the algorithms use. Path-equality checks and getParent() lookups in FillBadData, RemoveFlaggedFeatures and the shared fill utilities therefore match.
• Removed error codes (-78100, -9892, and -5557 in RequireMinimumSizeFeatures) have no remaining references in those filters' code, tests or docs.

Verdict

APPROVE: I found no correctness, lifetime or behavior defects. The earlier unit-test nit is fixed. What remains is a stale RemoveFlaggedFeatures V&V report and a one-character test nit, and the author can fix both in a follow-up commit.

[imikejackson]

@joeykleingers The two items from your last review are fixed in 00066ec.

• RemoveFlaggedFeatures V&V report: Code-path rows 29–31 now cover -9893, -9894 and the shared Cell Attribute Matrix paths. Row 1 (-9895) also lists the Strict Feature Ids sections. The two new tests are in the inventory, and row 149 now says that the -45437 test grows the geometry X dimension.
• Other inventory errors: Five tests from ENH: Out-of-core architecture rewrite and filter optimizations #1568, ENH/DOC: Add Better Preflight Feedback on Stale AMs #1748 and ENH: Send loop progress through the progress messaging API #1760 were not in the inventory. Three tests that the report called "retired" are still in the test file. The inventory now lists all of them, and the count is 28 active TEST_CASEs. ctest -R RemoveFlaggedFeatures passes 28/28.
• Bool literal: The Shared Cell Paths test now assigns false instead of 0.

[joeykleingers]

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

• The FillBadData Ignored Array Cell AM Alias test now runs through scope.executeFilter(), so each algorithm scenario is forced and checked.
• The RemoveFlaggedFeatures, ErodeDilateBadData and FillBadData V&V summaries and inventories now match the test files and path tables. The ErodeDilateBadData docs list -14605 and -14604.
• The FillBadData execute comment, the @note on both Cell Attribute Matrix validators, and the Ptr/Ref names on new pointers and references.

Verified locally on macOS arm64 (simplnx-Rel): the build has no warnings, and the 107 affected tests pass with both algorithm paths.

Problems that existed before this PR are not fixed here:

• #1843: QuickSurfaceMesh preflight crashes when the grid geometry has no Cell Attribute Matrix.
• FillBadData::AllBadData_TerminatesWithoutHang uses threshold 1, so it never reaches the no-progress check that V&V path row 16 says it covers.
• Several algorithm descriptions in the ErodeDilateBadData, FillBadData and RemoveFlaggedFeatures V&V reports describe older implementations.