Skip to content

REV: Reuse cell tuple validation in three filters - #1826

Closed
jmarquisbq wants to merge 28 commits into
BlueQuartzSoftware:developfrom
jmarquisbq:fix/cell-tuple-validation
Closed

jmarquisbq wants to merge 28 commits into
BlueQuartzSoftware:developfrom
jmarquisbq:fix/cell-tuple-validation

Conversation

@jmarquisbq

@jmarquisbq jmarquisbq commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Use the shared cell tuple-count validator in RequireMinNumNeighbors, WriteOnScaleTableFile, and PartitionGeometry. Keep the existing error codes, validation order, and count-only acceptance rules. Mismatch diagnostics identify the actual selected geometry and data paths, including linked objects and separately stored arrays or attribute matrices.

Add regression coverage for short, matching, and long counts; equal counts with different tuple shapes; linked selections; and errors that must take precedence over a count mismatch. Document the selected-path arguments on the protected validation helpers. The migration-only change covers 7 files, +363 / -49 lines.

Dependency and review scope

This is a follow-up to simplnx #1817, which supplies ValidateCellTupleCount. #1817 must merge before #1826. Until then, the full PR diff includes that prerequisite; the migration-only comparison isolates this change.

Standalone in-core validation uses public simplnx. The recorded OOC configuration uses DREAM3DNX with the merged DREAM3DNX #1121 wiring and the matching SimplnxOoc #6 branch for the inherited surface-metric HDF5 regression cases.

Test Plan

  • Scoped Windows Release builds pass for the in-core and OOC configurations.
  • The selected migration, helper, and surface-metric regressions pass: 48/48 in-core Release and 54/54 OOC Release.
  • The same in-core selection passes 48/48 in Debug with /bigobj for the pre-existing large test-translation-unit limit.
  • Changed C++ files pass clang-format 19.1.1 and whitespace checks.
  • CI passes on the configured platforms.

Related Issue

  • This PR has no related issue.

@imikejackson imikejackson left a comment

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.

Review of PR #1826 at 93d322fd2

This PR is stacked on #1817: 12 of its 13 commits are #1817, and the GitHub diff shows 21 files. I reviewed the migration-only range 008acec89..93d322fd2 (7 files, +363/−49) and separately reviewed #1817. Built SimplnxCoreUnitTest locally with the ci-macos-arm64 preset (Release) and ran the PartitionGeometryFilter, RequireMinNumNeighborsFilter, WriteOnScaleTableFileFilter, and ComputeSurfaceAreaToVolume tests serially: 47/47 pass, zero compiler warnings on the incremental build. clang-format 19.1.1 reports no changes on the changed C++ files.

Confirmed correct

  • The three call sites keep their existing error codes (-3009, -3010, -55571, -12024) and their position in the validation order. The -3010 RectGrid check still runs before dataCheckPartitioningMode, and the Image check still runs after the partition-count and mode checks; the two precedence tests prove it (-3012..-3014 and -3003..-3008 win over the count mismatch).
  • Linked selections are tested: the geometry and the matrix are reached through setAdditionalParent, and the error text is checked for the selected paths, not the storage paths. That was the stated goal and it holds.
  • Equal count with a different tuple shape ({4,2} vs {2,2,2}) is accepted in all three filters, matching the count-only contract of ValidateCellTupleCount.
  • RequireMinNumNeighborsFilter.md line 47 still describes -55571 accurately.
  • The replaced WriteOnScaleTableFile test is stronger than the one it removes (which only asserted invalid()).

Requested changes

1. (Blocking, process) Do not merge before #1817, then rebase so this PR is only the 7 migration files. The description already says this; I am making it the gate. I requested changes on #1817 (the ArraySelectionParameter root cause and a stale description), so this branch will need a rebase anyway. When you rebase, re-request review so the 7-file diff gets a clean look.

2. (Medium) The vertex-count branches are left in the old style, and one of them reuses an error code. dataCheckPartitioningScheme (line 693) still emits -3010 with the old humanName()-prefixed message for node geometries, so -3010 now means "RectGrid cell mismatch" in one branch and "vertex mismatch" in another. generateNodeBasedPSInfo (line 513) emits -3014, which is also the "Z dimension must be greater than 0" code at line 716. The collisions are pre-existing, but this PR's purpose is to unify tuple-count validation in this filter, and the new precedence test asserts on -3014 meaning the Z partition count. Please give the vertex mismatch its own code(s) and either route it through a sibling ValidateVertexTupleCount or format it the same way as the cell message.

3. (Low) #include <span> in PartitionGeometryTest.cpp is unused. The file uses nonstd::span. Drop it. (<iostream>/<ostream> are fine, std::cout was already used.)

4. (Low) Message style drift inside PartitionGeometryFilter. Every other error in this filter starts with "{}: " humanName(). The two migrated messages do not. Either accept that the shared validator has no prefix (and say so in its doc comment) or prefix at the call site. I lean toward the former, but be deliberate.

Notes, not blocking

  • The IWYU-style include additions (<any>, <atomic>, <memory>, <type_traits>, <utility>, <vector> in the filter sources and header) are harmless but add noise to a migration diff. Fine to keep.
  • No example pipelines or docs reference the old message text, so nothing else needs updating.

Verdict

REQUEST CHANGES for items 1 and 2.

Comment thread src/Plugins/SimplnxCore/test/PartitionGeometryTest.cpp Outdated
Use the perpendicular-axis face areas in the Direct and Scanline paths.
Add analytical 3D and single-slice tests for ratios and optional
sphericity. Preserve input data and existing archive controls.

Document the numerical correction for unequal X and Y spacing.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Require one Feature Id tuple per selected image cell before creating
output actions. Allow separately stored labels when the counts agree.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Use double precision for face areas, area sums, and volume arithmetic.
Keep float32 output arrays and the existing interface-selection rules.
Add connected-feature regressions with decimal and isotropic spacing.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Use double precision with a cube root of pi and a two-thirds exponent.
Check uniform scaling and preserve zero and negative volume behavior.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Describe zero-label interfaces, omitted exterior faces, supplied volumes,
and optional sphericity. Explain the numerical compatibility changes.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Return a preflight error for StringArray and NeighborList inputs before
reading Feature Id tuple metadata. Add tests for both input slots.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Use one count-only geometry check with caller-owned error codes and actionable paths. Cover image and rectilinear geometry counts without adding shape or ownership requirements.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Report the selected array kind and corrective action. Order the two new error codes and retain the existing codes.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Include standard-library dependencies explicitly. Reuse the current feature label and keep accumulation and loop syntax consistent.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
End the Scanline ratio buffer lifetime before the optional sphericity
allocation. Preserve bulk-write errors and reduce live feature-buffer
payload from 20 to 16 bytes per feature when sphericity is enabled.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Clarify voxelized sphericity and the X/Y spacing conditions. Add a
six-face schematic and a computed before-and-after comparison using
an explicitly labeled anisotropic copy of Small IN100.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Preserve the physical slice aspect while increasing panel width and
moving the shared legend below the views. Use the catalog Viridis
colors with a common zero-to-one sphericity scale.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Track the shared parameter validation follow-up in issue BlueQuartzSoftware#1833.
Keep the local guards while that compatibility change is developed.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Release label slices before metric processing. Reuse bounded count and
result buffers while retaining Float64 accumulation and Float32 outputs.
Test batch boundaries, analytical values, and later I/O failures.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Declare direct dependencies and use the configured naming conventions.
Preserve invalid-result details and make test buffer access explicit.
Keep the analytical expectations and scientific calculations unchanged.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Use the shared count-only check in RequireMinNumNeighbors,
WriteOnScaleTableFile, and PartitionGeometry. Preserve error codes
and validation order while reporting the actual selected paths.

Cover linked selections, separate data storage, unequal counts,
and equal counts with different tuple shapes.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
State that the selected geometry and data paths identify the inputs, so
the shared count diagnostic does not add a filter-name prefix.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Replace the unused standard span header with the direct nonstd span
dependency used by the test fixtures.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Reuse a local metadata check at both vertex-count validation sites.
Report selected paths, actual and expected counts, and corrective action
with error -3043. Preserve cell and partition-count error meanings.

Test linked selections, all four modes, and independent eight-vertex
counts, including separation from the Z partition-count error.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
@jmarquisbq
jmarquisbq force-pushed the fix/cell-tuple-validation branch from 93d322f to 5d8ef6b Compare October 19, 2026 19:47
Reject unsupported array kinds before typed access in PartitionGeometry,
RequireMinNumNeighbors, and WriteOnScaleTableFile. Keep optional inputs,
supported integer types, and existing validation order unchanged.

Track the temporary guards with issue BlueQuartzSoftware#1833. Test exact diagnostics,
unchanged data, inactive inputs, and error precedence. Preserve each
NeighborList fixture's AttributeMatrix shape. Correct configured include,
naming, and initialization findings on the migrated lines.

Signed-off-by: Jessica Marquis <jessica.marquis@bluequartz.net>
@jmarquisbq

Copy link
Copy Markdown
Contributor Author

The review follow-up is now published at dc54103a6:

  • Rebased the migration onto the updated BUG: Correct surface-area and sphericity calculations #1817 surface branch, preserving upstream tests and the writer's missing-directory behavior.
  • Gave vertex-count mismatches the dedicated -3043 diagnostic through a shared local helper. Messages report the selected paths and actual/expected counts, with corrective action (5d8ef6bd3).
  • Documented the shared cell validator's deliberate lack of a filter-name prefix (697a0a3d2).
  • Replaced the unused standard <span> include with the directly used nonstd header (a1f160eab).
  • Added temporary concrete-array guards in all three migrated filters, with comments tracking the parameter-level repair in BUG: Enforce array-kind contracts in ArraySelectionParameter #1833 (dc54103a6). The guards preserve inactive options, supported integer types, intentional StringArray phase names, and existing error precedence.
  • Added regression coverage for selected/linked paths, short/equal/long tuple counts, distinct vertex and partition errors, unsupported array kinds, unchanged data, and valid optional-input behavior. Corrected scoped include/naming/initialization findings and preserved fixture tuple-shape invariants.

Windows validation passes: 71/71 in-core Release, 78/78 OOC Release, and 71/71 in-core Debug. Both Release ALL_BUILD targets and the affected Debug targets pass. The OOC selection includes the actual disk-backed 200³ RequireMinNumNeighbors case. clang-format 19.1.1 passes. Configured clang-tidy diagnostics were reviewed; the raw runs retain documented Catch2/Result-macro diagnostics and an unchanged shared-header warning, with no actionable changed-code finding. Final adversarial reviews found no remaining actionable introduced finding.

All three inline review threads are addressed. #1817 must still merge first. The numerical algorithms are unchanged by this migration; Float64 accumulation and Float32 outputs remain intact. Current-head CI is pending.

imikejackson added a commit to imikejackson/simplnx that referenced this pull request Oct 20, 2026
The Image Geometry Existing Partitioning Scheme check used -3043, which
the node-geometry vertex tuple check from BlueQuartzSoftware#1826 also uses. It now uses
ValidateCellTupleCount, like the other PartitionGeometry tuple checks,
and returns -3045.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@imikejackson

Copy link
Copy Markdown
Contributor

Combined into #1840. The five commits that are not part of #1817 (232e708 through dc54103) were cherry-picked onto #1840's branch with authorship kept. To avoid error-code collisions in the combined branch and with #1829, the RequireMinNumNeighbors codes moved to -59571/-59574/-59575/-59576, and #1840's PartitionGeometry Existing Scheme check now uses ValidateCellTupleCount with -3045 so -3043 stays with the vertex check. Closing this PR in favor of #1840.

imikejackson added a commit to imikejackson/simplnx that referenced this pull request Oct 23, 2026
The Image Geometry Existing Partitioning Scheme check used -3043, which
the node-geometry vertex tuple check from BlueQuartzSoftware#1826 also uses. It now uses
ValidateCellTupleCount, like the other PartitionGeometry tuple checks,
and returns -3045.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
imikejackson added a commit to imikejackson/simplnx that referenced this pull request Oct 23, 2026
The Image Geometry Existing Partitioning Scheme check used -3043, which
the node-geometry vertex tuple check from BlueQuartzSoftware#1826 also uses. It now uses
ValidateCellTupleCount, like the other PartitionGeometry tuple checks,
and returns -3045.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
imikejackson added a commit that referenced this pull request Oct 23, 2026
)

* Check in preflight that grid-geometry cell inputs have one tuple per
  cell in 32 SimplnxCore and OrientationAnalysis filters. A short array
  was read past its end in execute; a long one was silently truncated.
  Read-only inputs may be in any Attribute Matrix if the count matches.
* Add a ValidateCellTupleCount overload that checks several DataPaths
  through IArray, so StringArray and NeighborList selections do not throw.
* Move the existing tuple checks in RequireMinNumNeighbors,
  PartitionGeometry and WriteOnScaleTableFile onto the shared helper and
  add array-kind checks (from #1826). RequireMinNumNeighbors codes move
  to -5957x to stay clear of #1829.
* Add a regression test and one documentation sentence per filter.
  NeighborOrientationCorrelation's test now runs the filter and checks
  which Attribute Matrix its repair copies affect.

Co-authored-by: Jessica Marquis <jessica.marquis@bluequartz.net>
Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants