Skip to content

VV: Compute C Axis Locations fully V&V'ed#1679

Open
JDuffeyBQ wants to merge 9 commits into
developfrom
vv/ComputeCAxisLocationsFilter
Open

VV: Compute C Axis Locations fully V&V'ed#1679
JDuffeyBQ wants to merge 9 commits into
developfrom
vv/ComputeCAxisLocationsFilter

Conversation

@JDuffeyBQ

Copy link
Copy Markdown
Collaborator

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

  • 1 Unit test to test output from the filter against known exemplar set of data
  • 1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

  • No commented out code (rare exceptions to this is allowed..)
  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added example pipelines that use the filter
  • Classes and methods are properly documented

@imikejackson imikejackson mentioned this pull request Jul 22, 2026
35 tasks
@imikejackson
imikejackson self-requested a review July 22, 2026 22:32

@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.

See comments for required changes.

@imikejackson

imikejackson commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

The core science checks out: I independently re-derived all 15 oracle fixtures by hand (v = gᵀ·[001], P = +1, z-flip) and all 15 are correct, and I reproduced the legacy comparison from scratch — for all-hexagonal input, 6.5.171 and NX are bit-identical on all 15 fixtures, and both are within 4.75e-8 of float64 analytic values. However, re-running the A/B with non-hexagonal cells in the mix shows real behavioral deviations that the deviations file currently denies, plus a handful of documentation/test items. Checkboxes below.

Deviations file / legacy comparison

  • vv/deviations/ComputeCAxisLocationsFilter.md — the headline "No deviations observed" is incorrect. 6.5.171 FindCAxisLocations takes only Quats (no Phases / CrystalStructures parameters) and computes a c-axis for every cell regardless of symmetry — its output on all-cubic input is bitwise identical to its output on all-hex input. A/B rerun with mixed hex/cubic phases (same 15 quats, phases alternating 1=Hex_High / 2=Cubic_High): hexagonal cells bit-identical, but cubic cells are computed values in 6.5.171 vs NaN in NX. With zero hexagonal ensemble entries, 6.5.171 produces full output while NX errors (-3522) and the pipeline aborts. Add structured entries, e.g.:
    • D1 — non-hexagonal cells: NaN (NX) vs meaningless-but-finite computed value (6.5.171). NX intentional improvement; no legacy fix warranted.
    • D2 — no hexagonal phases present: hard error -3522 (NX) vs silent full execution (6.5.171).
    • D3 — added warnings -3521 (preflight, unconditional) and -3523 (execute, mixed phases); legacy emits none.
  • Report "Legacy comparison" row and "Deviations from DREAM3D 6.5.171" section: scope the "bit-identical" claim to hexagonal cells / all-hexagonal input.

Code paths

  • The code-path table misses the unconditional -3521 preflight warning (ComputeCAxisLocationsFilter.cpp:105) — it fires on every preflight, including 100%-hexagonal data (confirmed in a pipeline run). So "6 of 7 paths" is stale; it's 8 paths. Either document + test it as path 8, or reconsider the warning itself (it tells users with perfectly valid data to go check their data; preflight can't inspect values, which is presumably why it's unconditional — but then the message should say so, or the -3523 execute warning alone may be enough).
  • No test exercises the mixed hex + non-hex combination — the only case proving the NaN branch doesn't disturb neighboring computed values in the same execution. Suggested TEST_CASE below (validated locally against this branch: passes, 49 assertions, and its inputs are exactly the mixed A/B dataset above).

ReferenceFrameNotes.rst

  • Location: the root docs/ tree is the dormant developer-doc build (SIMPLNX_BUILD_SPHINX_DOCS is OFF and marked "not supported"); nothing references the new file, so it will never build or ship. Suggest wrapping/python/docs/source/ (the built user docs) + a toctree entry.
  • Broken figures: the file references Images/AlCu-485-LabDCT.png / AlCu-485-WRONG.png / AlCu-485-Transformed.png (hyphens) but docs/Images/ does not exist. The actual files are wrapping/python/docs/source/Images/AlCu-485_LabDCT.png etc. (underscores) — currently orphaned there, which also argues for moving the doc next to them.
  • Pandoc-mangled tables: the representations table has a broken "A bb r." header column, and the filter table splits words mid-cell ("Rotate Euler R eference Frame", "R odrigues C onvertor"). Needs re-flowed column widths.

V&V report cleanup

  • At-a-glance Oracle row says "2 hand derived data fixtures" — it's 15.
  • Code-path "Source" line still contains scaffold placeholders: Filters/Algorithms/<ComputeCAxisLocationsFilter>.cpp (N lines) — actual file is Filters/Algorithms/ComputeCAxisLocations.cpp (107 lines).
  • Typos: "error branchs", "Handed derived", "forced to postive", "Superceded" → "Superseded", and the garbled sentence "The using the previous formula the exact form results were produced and compared against DREAM3D output."

Tests / conventions

  • No hexagonal phases error, Not all hexagonal phases warning, and Class 1 Oracle omit UnitTest::LoadPlugins(), and none of the new tests call UnitTest::CheckArraysInheritTupleDims(dataStructure) (project test conventions).
  • The oracle test's Approx(k_Fixtures[i].expectedOutput[j]) comparisons against expected 0.0f only pass because those components currently compute to exactly 0.0f; Approx(...).margin(1e-7f) makes them robust to future EbsdLib arithmetic changes.

User documentation

  • docs/ComputeCAxisLocationsFilter.md (plugin docs) doesn't mention the new behavior users will actually hit: NaN fill for non-hexagonal cells and the -3522 error when no hexagonal phase exists.
Suggested mixed-phase TEST_CASE (validated on this branch)
TEST_CASE("OrientationAnalysis::ComputeCAxisLocationsFilter: Class 1 Oracle - Mixed hexagonal and non-hexagonal phases", "[OrientationAnalysis][ComputeCAxisLocationsFilter]")
{
  DataStructure dataStructure;

  const usize size = k_Fixtures.size();

  Float32Array* quats = UnitTest::CreateTestDataArray<float32>(dataStructure, "Quats", {size}, {4});

  for(usize i = 0; i < size; i++)
  {
    for(usize j = 0; j < 4; j++)
    {
      quats->setComponent(i, j, k_Fixtures[i].inputQuat[j]);
    }
  }

  UInt32Array* crystalStructures = UnitTest::CreateTestDataArray<uint32>(dataStructure, "CrystalStructures", {3}, {1});
  crystalStructures->setValue(0, ebsdlib::CrystalStructure::UnknownCrystalStructure);
  crystalStructures->setValue(1, ebsdlib::CrystalStructure::Hexagonal_High);
  crystalStructures->setValue(2, ebsdlib::CrystalStructure::Cubic_High);

  // Alternate hexagonal (phase 1) and cubic (phase 2) cells so a single execution exercises
  // both the computed c-axis path and the non-hexagonal NaN path, and verifies the NaN
  // branch does not disturb neighboring computed values.
  Int32Array* phases = UnitTest::CreateTestDataArray<int32>(dataStructure, "Phases", {size}, {1});
  for(usize i = 0; i < size; i++)
  {
    phases->setValue(i, i % 2 == 0 ? 1 : 2);
  }

  DataPath cAxisLocationsPath({"CAxisLocations"});

  ComputeCAxisLocationsFilter filter;
  Arguments args;

  args.insertOrAssign(ComputeCAxisLocationsFilter::k_QuatsArrayPath_Key, std::make_any<DataPath>(std::vector<std::string>{"Quats"}));
  args.insertOrAssign(ComputeCAxisLocationsFilter::k_CellPhasesArrayPath_Key, std::make_any<DataPath>(std::vector<std::string>{"Phases"}));
  args.insertOrAssign(ComputeCAxisLocationsFilter::k_CrystalStructuresArrayPath_Key, std::make_any<DataPath>(std::vector<std::string>{"CrystalStructures"}));
  args.insertOrAssign(ComputeCAxisLocationsFilter::k_CAxisLocationsArrayName_Key, std::make_any<std::string>(cAxisLocationsPath.getTargetName()));

  auto preflightResult = filter.preflight(dataStructure, args);
  SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions);

  auto executeResult = filter.execute(dataStructure, args);
  SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result);

  REQUIRE(ContainsCode(executeResult.result.warnings(), -3523));

  REQUIRE(dataStructure.containsData(cAxisLocationsPath));
  auto& cAxisLocations = dataStructure.getDataRefAs<Float32Array>(cAxisLocationsPath);
  for(usize i = 0; i < size; i++)
  {
    const bool isHexCell = i % 2 == 0;
    for(usize j = 0; j < 3; j++)
    {
      INFO(fmt::format("i = {} | j = {} | phase = {} | input_quat = ({}) | expected_vec = ({})", i, j, phases->getValue(i), fmt::join(k_Fixtures[i].inputQuat, ", "),
                       fmt::join(k_Fixtures[i].expectedOutput, ", ")));
      if(isHexCell)
      {
        REQUIRE(cAxisLocations.getComponent(i, j) == Approx(k_Fixtures[i].expectedOutput[j]));
      }
      else
      {
        REQUIRE(std::isnan(cAxisLocations.getComponent(i, j)));
      }
    }
  }
}
A/B reproduction details (mixed and all-non-hex runs vs 6.5.171)
  • Shared input minted as a legacy-format .dream3d (ImageGeom 15x1x1; the 15 oracle quats bit-exact as float32; Phases; CrystalStructures), read by both runners.
  • 6.5.171 PipelineRunner: DataContainerReader → FindCAxisLocations → DataContainerWriter. NX nxrunner: ReadDREAM3D → ComputeCAxisLocations → WriteDREAM3D.
  • All-hex: CAxisLocations bit-identical (uint32-view equality), max |Δ| vs float64 analytic = 4.75e-8 for both.
  • Mixed (alternating Hex_High/Cubic_High): hex rows bit-identical; cubic rows finite values in 6.5.171 vs NaN in NX.
  • All-cubic: 6.5.171 output bitwise identical to its all-hex run (confirms legacy ignores phase entirely); NX fails preflight-clean but errors at execute with -3522 and writes no file.

JDuffeyBQ and others added 3 commits July 22, 2026 21:11
Summary:
* Confirmed no bugs (clean port)
* Confirmed no deviations from DREAM3D 6.5.171
* Retired 2 tests
  * Retired circular oracle test
  * One test superceded by more specific test
* Added Class 1 Oracle test
* Test data files replaced with 15 inline test fixtures
* Added V&V report, deviations
* Added ReferenceFrameNotes.rst to help explain how the filter
calculations rotations
* Providence file is not necessary due to test archive being
replaced by inline data

Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
* Comparison pipelines and results are working evidence, not source-tree
  deliverables; the distilled numbers belong in the vv/deviations entries
* Removes the ConvertOrientationsFilter pipelines and comparison report
  that should not have been committed

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
* Exercises the computed c-axis path and the non-hexagonal NaN path in a
  single execution using the 15 Class 1 Oracle quaternions with phases
  alternating Hexagonal_High / Cubic_High
* Verifies hexagonal cells match the hand-derived oracle values, cubic
  cells are NaN-filled, and warning -3523 is emitted
* Same inputs were used for the legacy 6.5.171 A/B comparison documenting
  the non-hexagonal behavioral deviations

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
@imikejackson
imikejackson force-pushed the vv/ComputeCAxisLocationsFilter branch from 19387ad to acc31a4 Compare July 23, 2026 01:11
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
@JDuffeyBQ
JDuffeyBQ requested a review from imikejackson July 23, 2026 14:55
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