Skip to content

VV: Update WriteDREAM3DFilter testing - #1683

Open
mmarineBlueQuartz wants to merge 4 commits into
BlueQuartzSoftware:developfrom
mmarineBlueQuartz:vv/WriteDream3dFilter
Open

VV: Update WriteDREAM3DFilter testing#1683
mmarineBlueQuartz wants to merge 4 commits into
BlueQuartzSoftware:developfrom
mmarineBlueQuartz:vv/WriteDream3dFilter

Conversation

@mmarineBlueQuartz

Copy link
Copy Markdown
Collaborator
  • Add V&V files for WriteDREAM3DFilter
  • Expanded round-trip testing for additional DataObject types.
  • New tests for untested code paths.

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

Full adversarial V&V review. I independently verified the report's claims against the tree at head 7d24b7d: all 17 Write-touching TEST_CASEs exist and pass locally (plus CI green on all 5 platforms), ProbeHdf5Dataset and the compression sub-suite are real, the error codes (-1/-2/-15), UUIDs, parametersVersion()==2, the FromSIMPLJson compression override, the 16 KiB doc threshold, and the cited PRs (#1606/#1544/#1253/#900) all check out. The 19-path enumeration is arithmetically consistent with the algorithm source, and the oracle is genuinely Class 1 and non-circular (all fixtures inline, no exemplar archive — so no provenance sidecar is correctly required).

Requesting changes for the items below. The blocking ones:

  1. Dead writeXdmf GENERATE in the IO test, which also poisons the report's Path 17 citation.
  2. The "every geometry and DataObject type" claim is false — RectGridGeom is never written by any test, and ScalarData/GridMontage are uncovered despite having IO classes.
  3. Leftover dead code from an abandoned DynamicListArray round-trip (the type has no HDF5 IO at all — worth recording in the report instead of silently dropping).
  4. NeighborList values are never compared after round-trip (only the tuple count).
  5. The use_compression=false FromSIMPLJson override the report leans on is not asserted by the SIMPL BC test.

On the legacy A/B question: I agree no legacy comparison is meaningful here — the two writers target deliberately different on-disk contracts, and the only tool that can read both formats is simplnx's own reader, which would make a writer comparison circular. The Class 1 round-trip + HDF5 layout probing is the right oracle. See the inline suggestion on D1 to say so explicitly.

Remaining inline comments are nits (tags, CheckArraysInheritTupleDims, naming, fixture geometry validity, unused locals, cleanup).

Comment thread src/Plugins/SimplnxCore/test/DREAM3DFileTest.cpp
Comment thread src/Plugins/SimplnxCore/test/DREAM3DFileTest.cpp Outdated
Comment thread src/Plugins/SimplnxCore/test/DREAM3DFileTest.cpp Outdated
Comment thread src/Plugins/SimplnxCore/test/DREAM3DFileTest.cpp Outdated
Comment thread src/Plugins/SimplnxCore/test/DREAM3DFileTest.cpp Outdated
Comment thread src/Plugins/SimplnxCore/vv/WriteDREAM3DFilter.md
Comment thread src/Plugins/SimplnxCore/vv/WriteDREAM3DFilter.md
Comment thread src/Plugins/SimplnxCore/vv/WriteDREAM3DFilter.md
Comment thread src/Plugins/SimplnxCore/vv/WriteDREAM3DFilter.md Outdated
Comment thread src/Plugins/SimplnxCore/vv/deviations/WriteDREAM3DFilter.md
Add V&V documentation
* Fixed DREAM3DIO checking the wrong result.
* Added ScalarData and RectGridGeom testing to DREAM3DFileTest.
* Improved NeighborList testing
* Updated geometries to use arrays of the appropriate tuple components.
* Check that the xdmf file exists.
* Added check that the use compression argument is disabled when importing from legacy SIMPL since it did not exist .
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.

3 participants