Skip to content

VV: Create Element Array from Feature Array fully V&V'ed - #1689

Open
imikejackson wants to merge 1 commit into
BlueQuartzSoftware:developfrom
imikejackson:vv/copy_feature_array_to_element_array
Open

VV: Create Element Array from Feature Array fully V&V'ed#1689
imikejackson wants to merge 1 commit into
BlueQuartzSoftware:developfrom
imikejackson:vv/copy_feature_array_to_element_array

Conversation

@imikejackson

@imikejackson imikejackson commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Full V&V of CopyFeatureArrayToElementArrayFilter per the v2 MTR V&V policy (Class 1 Analytical oracle + Class 4 Invariant companions), followed by a five-review hardening pass (adversarial, senior-engineer, CPU-perf, memory-perf, out-of-core). V&V report/deviations are archived to OneDrive per program decision.

Correctness

  • Class 1 analytical oracle (hand-derived indirection-lookup constants on a 4x3x1 / 4-feature fixture across float32/1-comp, int32/3-comp, and bool arrays) encoded as inline test assertions; SIMPLNX matched exactly.
  • A/B vs DREAM3D 6.5.171: numeric output bit-identical on valid input (re-verified after the kernel rewrite). 3 documented deviations, all naming/validation semantics: D1 output naming for converted pipelines; D2 over-provisioned feature array accepted in NX (legacy error -5555); D3 legacy bug — silent out-of-bounds garbage for negative feature ids (NX errors -5355).

Bug fixes

  • Selecting a NeighborList/StringArray threw an uncaught std::bad_cast from preflight — parameter now constrained to ArrayType::DataArray (pinned by test).
  • A / in the created-array suffix threw an uncaught std::invalid_argument — now a clean preflight error (-3021).
  • Zero-tuple FeatureIds dereferenced end() (UB) in the shared ValidateFeatureIdsToFeatureAttributeMatrixIndexing — empty-store guard added.

Performance

  • Raw-pointer fast path in the copy kernel when all three stores are concrete in-core DataStore<T> (removes ~3 virtual calls per component-element); OOC/other store types keep the serial virtual path (requireArraysInMemory added).
  • FeatureIds min/max validation hoisted out of the per-selected-array loop (was one full array scan per selected array).

Tests

  • 21 ctest entries (was 17), 13 of 14 enumerated code paths exercised (cancel path excluded). New negative tests: non-DataArray selection, / suffix, name collision (-266), zero-tuple FeatureIds, negative id (-5355), id-out-of-range (-5351), over-provisioned pin (D2). The TEMPLATE_LIST test previously compared uninitialized feature data (could not detect indexing bugs) — now uses distinct per-feature values. SIMPL backcompat test now asserts the converted multi-path value.

Docs

  • Xmdf→Xdmf typo, suffix/multi-array naming semantics, validation error codes, memory note, example pipelines.

Test Plan

  • All 21 filter tests pass on in-core build
  • All 21 filter tests pass on out-of-core build configuration
  • Full SimplnxCore suite green in-core (one pre-existing unrelated failure: EMsoftSO3SamplerFilter missing SIMPL fixture in SimplnxReview)
  • A/B comparison vs 6.5.171 re-run post-rewrite: bit-identical

@imikejackson
imikejackson requested a review from nyoungbq July 24, 2026 01:39
@imikejackson
imikejackson force-pushed the vv/copy_feature_array_to_element_array branch from 7fa565f to af11360 Compare July 24, 2026 01:39
@imikejackson imikejackson changed the title VV/PERF: Create Element Array from Feature Array fully V&V'ed VV: Create Element Array from Feature Array fully V&V'ed Jul 24, 2026
nyoungbq
nyoungbq approved these changes Jul 28, 2026
View reviewed changes
Comment thread src/Plugins/SimplnxCore/vv/CopyFeatureArrayToElementArrayFilter.md
Comment on lines +108 to +129
## Post-V&V hardening (five-review pass, 2026-07-23)

Five independent reviews were run after the standard workflow: adversarial, nit-picky senior engineer, CPU performance, memory performance, and out-of-core. Actions taken:

**Fixed (code):**
1. *(Adversarial, Critical)* Selecting a NeighborList/StringArray crashed preflight with uncaught `std::bad_cast` — parameter now constrained to `ArrayType::DataArray`; pinned by test.
2. *(Adversarial)* The empty-selection guard was untested (old `Parameter Check` failed on the FeatureIds parameter before reaching the guard) — test reworked to exercise the guard itself.
3. *(Adversarial)* Suffix containing `/` threw uncaught `std::invalid_argument` — preflight now rejects with -3021; pinned by test.
4. *(Adversarial)* Zero-tuple FeatureIds dereferenced `end()` (UB) in `ValidateFeatureIdsToFeatureAttributeMatrixIndexing` — empty guard added in the shared utility; pinned by test.
5. *(CPU/Memory/OOC, consensus)* FeatureIds min/max validation ran once per selected array (K full array scans) — hoisted to a single call (valid because preflight -3020 guarantees equal tuple counts).
6. *(CPU perf)* Copy kernel did ~3 virtual calls per component-element via `AbstractDataStore`/`ValueProxy` (~10-30× slower per thread than the legacy memcpy gather) — added a raw-pointer fast path taken when all three stores are concrete in-core `DataStore<T>` (`std::copy_n` per tuple, cancel check every 65536 tuples), with the virtual path kept as the OOC/other-store fallback. This also resolves the thread-safety review question: parallel threads write disjoint ranges of a raw buffer on the fast path; non-`DataStore<T>` stores fall back to the virtual path, and `requireArraysInMemory()` + the OOC auto-serialization keep that path serial.
7. *(Senior)* Naming/style/test-hygiene items: `k_SelectedFeatureArrayPaths_Key` rename (JSON key unchanged), `Xmdf`→`Xdmf`, `tDims`→`tupleShape`, `p...Value` prefix consistency, const `InputValues` ctor param, doxygen rewrite, `REQUIRE_NOTHROW` before every `getDataRefAs`, `CAPTURE(i)` in assertion loops, tag standardization to `[SimplnxCore]`, magic-number cleanup, removed unused `<fstream>`, backcompat test now asserts the converted multi-path value.
8. *(Memory/docs)* Docs now state the per-array Element-sized memory cost and the -3020/-3021 error codes.

**Corrected finding:** the adversarial review predicted the name-collision case would error at *preflight* (-266); empirically `IFilter::preflight` does not apply output actions, so the error surfaces at execute (or pipeline-level preflight in the GUI). The test pins the actual behavior and that the source array is untouched.

**Deferred / escalated:**
- *(OOC, Critical — infrastructure, not this filter)* The `simplnx-ooc-Rel` build registers no OOC backend, so its "OOC" test runs are actually in-core (see At a glance caveat). Needs a build-infrastructure decision (FileStore source dir vs SimplnxOoc wiring).
- *(OOC)* Slab/bulk-I/O restructure and feature-array local caching for the OOC rewrite branch's chunked stores — deferred to the OOC architecture rewrite, where `copyIntoBuffer`/`copyFromBuffer` exist.
- *(OOC)* Tier-1 200³ OOC test — deferred until the OOC backend gap is fixed (it would prove nothing today).
- *(Memory, framework-wide)* `CreateArrayAction` zero-fills every created store before the kernel overwrites 100% of it (double touch) — framework enhancement request, not filter-specific.
- *(Adversarial, framework-wide)* `MultiArraySelectionParameter` stores but never enforces `AllowedDataTypes`; the `ArrayType` constraint is what protects this filter.

@nyoungbq nyoungbq Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to consolidate this if possible

@imikejackson
VV/PERF: Create Element Array from Feature Array fully V&V'ed
ee3d0a6 
Summary:
- Found and fixed 3 bugs (uncaught std::bad_cast in preflight when a
  NeighborList/StringArray was selected - parameter now constrained to
  ArrayType::DataArray; uncaught std::invalid_argument for a '/' in the
  created-array suffix - now clean preflight error -3021; end() iterator
  dereference (UB) on zero-tuple FeatureIds in the shared
  ValidateFeatureIdsToFeatureAttributeMatrixIndexing utility);
- documented 3 deviations from DREAM3D 6.5.171 (D1 output-array naming for
  converted pipelines, D2 over-provisioned feature array accepted in NX vs
  legacy error -5555, D3 legacy bug: silent out-of-bounds garbage for
  negative feature ids vs NX error -5355) - numeric output bit-identical
  on valid input across float32/int32x3/bool fixtures;
- retired 1 test (Parameter Check exercised parameter validation instead of
  the filter's empty-selection guard; replaced with a guard-specific test);
- augmented existing tests with 6 inlined Class 1 (Analytical) + Class 4
  (Invariant) test fixtures (21 ctest entries total, 13 of 14 code paths;
  cancel path excluded);
- V&V deliverables (report, deviations) archived to OneDrive per program
  decision; no exemplar archive needed (all fixtures in-memory);
- PERF: raw-pointer fast path for in-core DataStore<T> in the copy kernel
  (removes ~3 virtual calls per component-element), FeatureIds min/max
  validation hoisted out of the per-selected-array loop, cancel check
  throttled; OOC and other store types keep the serial virtual path;
- fixed Xmdf->Xdmf typo, documented error codes, memory note, and example
  pipelines in the user-facing doc.

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
@imikejackson
imikejackson force-pushed the vv/copy_feature_array_to_element_array branch from af11360 to ee3d0a6 Compare July 28, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nyoungbq nyoungbq nyoungbq approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@imikejackson @nyoungbq