VV: Create Feature Array From Element Array Fully V&V'ed

[nyoungbq]

No description provided.

[imikejackson]

V&V PR Review — CreateFeatureArrayFromElementArrayFilter

Reviewed at 4dd2d3a57. Every claim in the three V&V documents was checked against the tree, against the SIMPL 6.5.171 source, and against an independent A/B re-run on this machine (SIMPL PipelineRunner 6.5.171 vs nxrunner built from this branch). Verification notes follow the checklist.

The headline: the "no deviations / bit-identical / no migration action required" conclusion is only true for the single configuration that was tested. 6.5.171 has two hard error gates that SIMPLNX silently replaced with an AttributeMatrix resize, and that resize destroys sibling arrays. Reproduced empirically below.

---

Blocking

• Deviations doc is wrong: 6.5.171 has two hard error gates that SIMPLNX dropped. SIMPL/Source/SIMPLib/CoreFilters/CreateFeatureArrayFromElementArray.cpp (~lines 236–266) refuses to run and errors out when the Feature AM is not exactly max(FeatureIds)+1 tuples: -5555 ("Attribute Matrix ... has N tuples but the input array ... has a Feature ID value of at least M") and -5556 ("The number of Features in the InArray array (N) does not match the largest Feature Id"). SIMPLNX unconditionally calls resizeTuples({maxValue + 1}) instead (Algorithms/CreateFeatureArrayFromElementArray.cpp:90-91). Empirically confirmed on identical input:

  Feature AM tuples	SIMPL 6.5.171	DREAM3D-NX
  3 (== max+1)	exit 0, matches oracle	exit 0, matches oracle
  1 (< max+1)	exit 1, Error -5555	exit 0, resizes AM 1→3
  5 (> max+1)	exit 1, Error -5556	exit 0, shrinks AM 5→3

  Add CreateFeatureArrayFromElementArray-D1 / -D2 entries, drop the "No deviations" headline, and change the Migration recommendation — "No migration action required" is not correct: a 6.5.171 pipeline that errored out now silently succeeds with a mutated AM.

• Silent data loss: the AM resize truncates sibling feature arrays. AttributeMatrix::resizeTuples fans out to every child IArray (src/simplnx/DataStructure/AttributeMatrix.cpp:114-122). In the oversized case above, an unrelated sibling array Active in the same CellFeatureData AM was silently truncated from [1,2,3,4,5] to [1,2,3]. In the undersized case it was grown from [1] to [1,0,0]. The filter mutates data it was never asked to touch. Report says "Bug flags: None identified during code analysis" — this needs a bug flag and a decision (error out like legacy, or restrict to grow-only + validate).

• usize maxValue overflow wipes the whole Attribute Matrix. Algorithms/…cpp:85-86 casts a possibly-negative int32 max into usize; maxValue + 1 then wraps. Reproduced with FeatureIds = [-1]*8: NX resized CellFeatureData to 0 tuples, taking every array in it with it, then failed downstream in the writer with a misleading -1305 BuildChunkedDeflateDcpl encountered a zero-valued dimension. 6.5.171 cleanly errors -5556 on the same input. Needs a guard (preflight can't see the data, so validate in operator() before the resize).

• Material-PR claim for #1278 is false. Report: "#1278 — ... FeatureId range validation added to preflight." gh pr diff 1278 shows the only change to this filter was the k_CellFeatureIdsArrayPath_Key help string and its default DataPath. There is no FeatureId range validation in preflightImpl today (CreateFeatureArrayFromElementArrayFilter.cpp:81-113 creates one array and nothing else). Either remove the claim or actually add the validation.

• A/B is not reproducible as submitted. The provenance doc points the A/B artifacts at /home/nyoung/Apps/DREAM3DNX-Dev/feature_from_element_vv/ — unreachable to any reviewer, with no OneDrive archive reference. Precedent exists for committing these: src/Plugins/SimplnxCore/vv/comparisons/RotateSampleRefFrameFilter/ holds make_input.py, gen_simpl_pipeline.py, gen_nx_pipeline.py, results.md. Please do the same here (or cite the archive path).

• Widen the A/B beyond the exactly-sized AM. The only fixture tested was the one case where the two implementations agree. Minimum additional cases: undersized AM, oversized AM, all-negative FeatureIds, FeatureIds with a gap. My fixtures/pipelines/logs for all of these are in Workspace4/ww_work/CreateFeatureArrayFromElementArray_PR1695_review/ — reuse them if helpful.

---

Report accuracy

• Port-time delta #1 is wrong about legacy. Report: "QVector<int32_t> + linear search → std::map<int32, usize>". Legacy actually used QMap<int32_t, T*> featureMap (SIMPL source line 181) — already an associative container, storing a pointer to the first-seen tuple rather than an index. There was no linear search.

• "Algorithm Relationship: Port — line-for-line translation ... Identical control flow" is not supportable. Two error gates were removed and an AM resize was added. That is a behavioral change, i.e. Minor at least, not a pure Port.

• Claimed Class 4 invariant featureAttributeMatrix.shape() == {max(featureIds)+1} is not asserted anywhere. The AF tests only assert outArray.getNumberOfTuples() == 3. (CheckArraysInheritTupleDims covers it indirectly, but the code-path table row #1 says "output AM has correct shape asserted via Class 4 invariant" — either add the explicit assertion or reword.)

• Claimed invariants outputArray.getDataType() == inputCellArray.getDataType() and ...getNumberOfComponents() == inputCellArray... are not what the tests do. The tests compare against hardcoded literals (DataType::float32, == 1). Assert against the input array so the invariant is actually the invariant.

• Second-engineer skip rationale is circular. Provenance doc: "A wrong oracle would produce a test failure inconsistent with the SIMPL regression baseline, providing external cross-validation." AF-1/2/3 are never run through SIMPL — the A/B used entirely different 8×1×1 fixtures. No such cross-validation exists; drop that sentence or run the AF fixtures through 6.5.171.

• Status = COMPLETE while Sign-off = <pending>. Precedent (vv/FillBadDataFilter.md) uses READY FOR REVIEW in exactly this state.

• Summary says "All six oracle assertions pass" — the three AF test cases contain 27 REQUIREs. Fix or drop the count.

• Algorithm cpp is described as "95 lines" (twice); it is 94. The copy loop is cited as "lines 26–55" / "lines 43–55"; it is lines 25–56.

• Material PR titles for #1301 and #1544 are paraphrased. Actual: "ENH: Move Execution to Algorithm Classes" and "ENH: Move non-trivial Filter executeImpl() logic to Algorithm classes".

• Deviations doc RGB fixture description ("cells 0,2,4,6→[10,20,30]/[70,80,90] interleaved with cells 1,3,5,7→[40,50,60]") is ambiguous — it takes a reconstruction pass to recover the 8 actual tuples. List them explicitly.

• At-a-glance table is missing the SIMPLNX Human Name row that the other vv/ reports carry.

---

Test coverage gaps

• No test for a gap in the FeatureIds range (e.g. featureIds = [2,2] → feature 1 never written). AF-2 only ever checks tuple 0, which existed at creation time and so was covered by the CreateArrayAction fill. Tuples added by resizeTuples get their value from DataStore::m_InitValue, and ArrayCreationUtilities.hpp sets that only for the in-core DataStore<T> ("Only base data store has initialization value"). So the "never written → 0" claim is untested for grown tuples and is store-implementation dependent. Add the fixture.

• No preflight/error test at all. Nothing exercises a failure path, and the code-path enumeration ("5 of 6") only covers the Algorithm class — preflightImpl isn't enumerated, nor is the AM-shrink path, the overflow path, or the empty-featureIds path. The denominator is wrong, not just the numerator.

• Unvalidated precondition: FeatureIds tuple count is never compared to the selected cell array's. The loop bound is selectedCellStore.getNumberOfTuples() (Algorithms/…cpp:24) but featureIds[cellTupleIdx] is indexed with it unchecked; nothing forces the two arrays into the same AM. A smaller FeatureIds array is an out-of-bounds read. Legacy shares this hole so it is not a deviation, but it contradicts "Bug flags: None". Cheap preflight fix.

• Negative FeatureId with a positive max is an unchecked wild write. createdDataStore[totalCellArrayComponents * featureIdx + cellCompIdx] with featureIdx = -1 promotes to SIZE_MAX; DataStore::setValue is m_Data.get()[index] = value with no bounds check (DataStore.hpp:297-300). Reproduced with FeatureIds = [-1,2,1,2,1,2,1,2]: both binaries "succeeded" with correct visible output while corrupting memory. Shared pre-existing hazard, but again — not "no bug flags".

• testElementArray still hand-rolls its comparison (if(oldVal != newVal) { REQUIRE(...); break; }) instead of UnitTest::CompareDataArrays<T>. The commit message claims the test code was cleaned up and the report explicitly calls this pattern out — worth actually replacing.

---

CPU / memory / out-of-core

• Redundant full reallocation of the output array. Algorithms/…cpp:90 resizes createdArrayStore, then line 91 resizes the parent AM — which resizes that same array again (it is a child of the AM). One of the two is a wasted allocate-and-copy of the whole output array. Drop line 90.

• std::map<int32, usize> is the wrong container here. One heap node per distinct feature, O(log F) per cell, and three lookups per cell (contains, operator[]=, operator[]). maxValue is already computed in operator() before dispatch — pass it into the functor and use a flat std::vector indexed by feature id: O(1), one contiguous allocation, no tree walk. This is the dominant cost in a filter that is otherwise a memcpy.

• OOC-hostile read pattern. selectedCellStore[firstInstanceCellTupleIdx + cellCompIdx] re-reads the first-seen cell of a feature for every later cell of that feature — an arbitrarily far-back random read that thrashes the chunk cache on a chunked store. Provably equivalent alternative: compare against the previous value for that feature (already sitting in createdDataStore[featureIdx * C + comp], guarded by a visited flag). For any value sequence, the first index differing from the first-seen value is the same index as the first differing from its predecessor — so the warning fires on the identical cell — and the input is then read strictly sequentially, exactly once.

• Input is read twice per component. selectedCellStore[totalCellArrayComponents * cellTupleIdx + cellCompIdx] appears at both line 46 and line 54. Hoist to one read.

• std::max_element result is re-indexed instead of dereferenced. Algorithms/…cpp:85-86 computes a std::distance then does featureIdsRef[featureIdsMaxIdx] — an extra random read (an extra chunk fetch out-of-core). Just *std::max_element(...). The same line also mixes .begin() with .cbegin()-derived iterators for no reason. This PR touched this line as part of a stated cleanup pass.

• std::max_element on an empty FeatureIds store dereferences end() — featureIdsMaxIdx becomes 0 and featureIdsRef[0] reads a 0-tuple store. Guard it.

---

Style and naming

• firstInstanceCellTupleIdx (line 42) and the featureMap value (line 38) hold totalCellArrayComponents * cellTupleIdx — a flat element index, not a tuple index. Rename (firstInstanceFlatIdx).
• maxValue (line 86) is the maximum feature id — maxFeatureId reads correctly. featureIdsMaxIdx goes away entirely with the fix above.
• Test: AnalyticalFixtures::k_InputCellPath is built from Constants::k_FaceData, i.e. the literal "FaceData" — so the cell input array inside a "Cell Data" AM is named "FaceData". Everywhere else in the tree k_FaceData names a face AM on a triangle geometry. Use a descriptive literal such as "InputCellArray".
• Test: k_ParentDGName / k_ParentDGPath — the DG abbreviation isn't used elsewhere in the plugin's tests; spell it out.
• Test: the em-dash in AnalyticalFixtures — AF-1 … is the only multibyte character in any TEST_CASE name in this plugin, and it makes ctest -R filtering awkward. Use AF-1: or a plain hyphen.
• User doc docs/CreateFeatureArrayFromElementArrayFilter.md doesn't mention that the filter resizes the Feature Attribute Matrix (and everything in it), the inconsistent-value warning, or the zero-fill of unmapped feature slots. Since this V&V surfaced the resize as a deviation, the doc should say so.

---

Verified correct

Recording what was checked and passed, so it doesn't get re-litigated:

• All 6 ctest entries pass individually and as a set (ctest -R CreateFeatureArrayFromElementArray, build NX-Com-Qt69-Vtk96-Rel). Dropping UnitTest::LoadPlugins() from the two regression cases does not break them in process isolation.
• clang-format 15.0.4 -style=file --dry-run -Werror is clean on both changed source files.
• UUIDs correct: SIMPLNX 50e1be47-b027-4f40-8f70-1283682ee3e7 (…Filter.hpp:120), SIMPL 94438019-21bb-5b61-a7c3-66974b9a34dc.
• SHA512 for 6_5_test_data_1_v2.tar.gz matches src/Plugins/SimplnxCore/test/CMakeLists.txt:220.
• #1295 claim verified: fill "0" added to the CreateArrayAction and the explicit createdDataStore.fill(0) removed; setInitValue is set for the in-core store.
• Warning-parity claim verified empirically — both 6.5.171 and NX emit exactly one -1000 warning per execution with identical text. Legacy's guard is bool warningThrown (SIMPL line 182), NX's is result.warnings().empty(); behaviourally equivalent as claimed.
• AF-1/AF-2/AF-3 hand derivations are arithmetically correct, and on the exactly-sized AM fixture both binaries matched my independent oracle (FeatureFloat = [0, 30, 20], FeatureRGB = [[0,0,0],[70,80,90],[40,50,60]]).
• Legacy zero-init confirmed: SIMPL DataArray::allocate() uses new T[n](), so legacy's feature-0 slot really is 0 — the A/B's [0.0, 30.0, 20.0] oracle is right.
• No parametersVersion() bump needed — parameters are unchanged by this PR.
• The circular-oracle flagging of the two pre-existing regression tests is correct, and disclosing it rather than deleting the tests is the right call.