VV: Write Image (ITK) fully V&V'ed

[JDuffeyBQ]

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]

V&V Review: Write Image (ITK)

Reviewed as a second-engineer V&V review: every claim in vv/ITKImageWriterFilter.md and vv/deviations/ITKImageWriterFilter.md was re-derived, the A/B claim was re-run with an independent methodology, and adversarial / CPU / memory / out-of-core passes were performed. Findings are grouped by severity.

1. Claims that verify — no action needed

• uint32/uint64 dispatch fix is real and is covered. Independent A/B: reverted CopyTupleTyped<uint32>→<int32> and <uint64>→<int64>, rebuilt, ran ctest -R "Analytical Pixel Order" → tests 957 (uint32) and 959 (uint64) fail; the other 8 pass. Restored the fix → 10/10 pass. The pre-fix failure mode is an uncaught std::bad_cast escaping filter.execute() (Catch2 reports {Unknown expression after the reported line} at ITKImageWriterTest.cpp:159), not a wrong-value result.
• Oracle values are correct. All seven expected slice matrices re-derived from value(x,y,z)=x+10y+100z on the 3×2×2 fixture (XY 3×2 ×2 slices, XZ 3×2 ×2 slices, YZ 2×2 ×3 slices) match the test source exactly. Max value 112 fits every type including int8, so no silent saturation.
• Port fidelity of the slicing arithmetic. The XY/XZ/YZ index expressions and indexNew in ITKImageWriterFilter.cpp:419-490 are byte-for-byte equivalent to 6.5.171 ITKImageWriter.cpp:396-449 + copyTuple().
• Deviation D1 verified against legacy source. 6.5.171 ITKImageWriter::saveImageData() does out << "_" << slice; with no padding, guarded by the same if(maxSlice != 1). The stated remediation (Total Number of Index Digits = 1) reproduces the legacy filename shape.
• 15/15 ctest entries pass on PR head (ctest -R ITKImageWriter, 1.40 s).
• No circular oracle, no new exemplar archive — inline expected values only, consistent with policy.

2. Blocking — undocumented behavioral deviations from 6.5.171

• B1. RGBA (4-component) input is a functional regression from 6.5.171, and the docs still advertise ARGB. Legacy compiled with DREAM3D_USE_RGB_RGBA 1 / DREAM3D_USE_Vector 0, i.e. itk::RGBPixel/itk::RGBAPixel. NX uses ITK::ScalarVectorPixelIdTypeList → scalar + itk::Vector, which has no nComp == 4 arm. Measured on a 4×3×2 uint8 geometry:

  nComp	preflight	execute
  1	valid	valid
  3	valid	valid (written as itk::Vector<T,3>, not itk::RGBPixel<T>)
  4	valid	fails, -21010 "Vector dimension not supported. cDims[0] = 4"

  docs/ITKImageWriterFilter.md:9 still says "grayscale, RGB or ARGB color values" and lines 15-16 direct users to Compute IPF Colors as valid input. Needs: a -D2 deviation entry, a component-count check in preflightImpl so this fails at preflight rather than mid-execute, and a docs correction.

• B2. XZ and YZ output files carry the wrong pixel spacing and origin. ITK::ImageGeomData(imageGeom) captures (sx,sy,sz)/(ox,oy,oz); executeImpl overwrites only .dims, so WrapDataStoreInImage always emits axis0 = sx/ox and axis1 = sy/oy regardless of plane. Measured with spacing (1,2,4) and origin (10,20,40), reading the .mha headers:

  Plane	DimSize	ElementSpacing	should be	Offset	should be
  XY	3 2	1 2	1 2 ✅	10 20	10 20 ✅
  XZ	3 2	1 2	1 4	10 20	10 40
  YZ	2 2	1 2	2 4	10 20	20 40

  This is also a deviation from 6.5.171, which built a fresh ImageGeom and set only dimensions (so it wrote spacing 1 1 / origin 0 0). Needs a fix plus a deviation entry. The oracle asserts pixels only, so it cannot catch this.

3. Correctness / robustness the V&V did not reach

• C1. Out-of-core input array → uncaught std::bad_cast. This filter hand-rolls its execute and never calls ITK::DataCheck/ITK::Execute, so it never reaches the k_OutOfCoreDataNotSupported (-2002) guard added by ENH: Again require in-memory data for ITK filters #1555 (ITKArrayHelper.hpp:843,865). With an OOC array, currentData.createNewInstance() returns an H5Store<T> (FileStore.hpp:138 → AbstractDataStore<T>), and WriteImage() does dynamic_cast<DataStore<T>&>(dataStore) → throws. Identical failure mode to the uint32 bug reproduced above. The report has no out-of-core section; ENH: Again require in-memory data for ITK filters #1555 is cited in the PR list but its guard was never verified to apply here.

• C2. A one-character Fill Character can throw out of preflightImpl. CreateIndexString builds fmt::format("{{:{}>{}}}", fillCharacter, totalDigits). Reproduced:

  • fillChar = "{" → fmt::format_error: invalid fill character '{' escapes preflight (in the GUI this is a keystroke away).
  • fillChar = "/" → accepted; preview becomes s_//3.tif, and execute writes to a mangled path.

  The size() != 1 fix validates length only. Validate that the character is a legal fmt fill and a legal filename character. Note executeImpl uses std::setfill(fillChar[0]) (iostreams) while preflight uses fmt — two different formatting paths for the same parameter.

• C3. The Example Output File preflight preview is wrong. CreateIndexString(maxSlice, ...) formats the slice count as the index and ignores indexOffset. Reproduced on a 3-slice XY export: preview s_003.tif, actual files s_000.tif, s_001.tif, s_002.tif — index 003 is never written. For a single-slice output it previews name_001.ext while the file is name.ext, contradicting the sentence this PR added to the docs. Should be CreateIndexString(indexOffset, ...) with the suffix suppressed when maxSlice == 1. Code path feature/infrastructure #1 ("Valid preflight") is claimed covered, but no test asserts this preflight value. (Same defect exists in SimplnxCore/WriteImageFilter.cpp:290 — worth a follow-up issue.)

• C4. copyFrom()'s Result<> is discarded. CopyTupleTyped (ITKImageWriterFilter.cpp:178) drops the return value, so a -14600/-14601 would silently produce wrong output instead of an error.

• C5. Dead code containing a live off-by-one. executeImpl always sets newImageGeom.dims Z = 1, so ArraySwitchFuncDimsImpl always selects Dimensions == 2, making if(Is2DFormat(filePath) && Dimensions == 3) unreachable. Is2DFormat(), WriteAs2DStack() and error -21012 are therefore all dead. WriteAs2DStack also contains for(uint64 index = indexOffset; index < (z_size - 1); index++) (writes one file too few, and starts numbering at indexOffset) plus a hardcoded {:03d} that ignores totalDigits/fillChar. Delete it, or state the unreachability explicitly in the report.

• C6. No cancel check and no progress messages. shouldCancel and messageHandler are unused in executeImpl. IFilter::execute only samples shouldCancel after executeImpl returns (IFilter.cpp:225), so a large multi-slice export is uninterruptible and silent. VV_Notes exempts testing the cancel path, not omitting the check.

4. CPU and memory review

• P1. Per-tuple type dispatch dominates runtime (~6.7× avoidable). CopyTuple performs a switch on DataType + two dynamic_casts + a virtual copyFrom per tuple. Micro-benchmark, 4,444,713 tuples (Small IN100 189×201×117), float32, this machine:

  per-tuple dispatch = 208 ms    hoisted dispatch = 31 ms
  

  Whole-filter times on the same volume: XY 268 ms, XZ 286 ms, YZ 339 ms — so the avoidable dispatch is roughly 60–75% of total runtime. Hoist the cast out of the slice loops. Better: XY slices are fully contiguous in the source store (one bulk copyFrom(0, src, slice*dA*dB, dA*dB) per slice replaces dA*dB calls) and XZ rows are contiguous per Z.
• P2. Full-volume temp buffer where legacy allocated one slice. currentData.createNewInstance() (DataStore.hpp:521) allocates getTupleShape() — the entire volume — zero-filled, then only one slice is ever populated. Legacy's ITKIW_PREP_DC allocated numElements = dA * dB. Small IN100 float32: 17.8 MB temp vs legacy's 152 KB (~117×), doubling peak footprint for the array. A 1000³ uint16 volume goes from ~2 GB + 2 MB to ~4 GB, plus a full-volume memset on every run. Allocate the slice shape instead. Neither the report nor the PR mentions this port-time regression.
• P3. nComp is dead. Passed to CopyTupleTyped and used only inside the #if 0 block. Remove both the parameter and the #if 0 block (ITKImageWriterFilter.cpp:172-176) per the PR's own "no commented out code" checklist.

5. Report and documentation accuracy

• R1. Test name mismatch. The report references Analytical TIFF Pixel Order in the Oracle "Encoded" line, in all six code-path rows, and in the test inventory. The actual test is Analytical Pixel Order. "TIFF" also misleads, since 9 of the 10 types write MetaImage.
• R2. "17 of 18 branches exercised" is understated. Not enumerated: component-count dispatch (nComp 1/2/3/10/11/36) and -21010; WriteAs2DStack/-21012 (dead, see C5); create_directories and -19000; AtomicFile::Create failure; CopyTuple's default: throw std::runtime_error; the preflight plane default:; and the OOC path (C1). Please recount, or state the denominator's scope explicitly.
• R3. Code path 17 and the test name misstate the mechanism. 3D Image Single-File Output produces one file because maxSlice == 1 in SaveImageData (dims.getY() == 1 for the XZ plane) — not because the format is "non-2D". Is2DFormat is never consulted (C5). Suggest renaming to e.g. Single Slice Keeps Exact Output Name and rewording the row.
• R4. "Minor changes … the pixel comparisons show no output change from these substitutions" is too strong. Pixels match, but the ITK-API substitution changed supported pixel types (B1) and the file metadata written (B2). Please reword the Algorithm Relationship section.
• R5. vv/provenance/ITKImageWriterFilter.md is missing. Every other V&V'd filter in the tree has one (SimplnxCore/vv/provenance/, OrientationAnalysis/vv/provenance/). The report's legacy-comparison claim — "Small IN100 has 117 XY, 201 XZ, and 189 YZ float32 TIFF slices", "each slice matches exactly" — ships with no pipelines, no comparison script, no hashes, no stated tolerance, and no OneDrive pointer, so it is not independently verifiable. Please add the provenance deliverable.
• R6. No Algorithm review section. OrientationAnalysis/vv/ReadAngDataFilter.md has one; this report has no algorithm review, CPU, memory, or out-of-core discussion — which is where P1, P2, and C1 would have surfaced.
• R7. PR history omissions. git log --follow on the .cpp/.hpp also lists Added versioning to filter parameters and json #1088 "Added versioning to filter parameters and json" (omitted, and relevant since the report says nothing about parametersVersion()), plus docs-only PRs DOC: Add standardized ChoicesParameter descriptions to filter docs #1571, ENH: Rename Filters that start with Find/Generate/Calculate to Compute #956, DOC: Fix capitalization issue in the documentation for the GitHub link. #975, DOC: Update all doc files to have correct filter human name #914, DOC: Link directly to the discussion page on GitHub. #847, DOC: Update documentation files to allow Sphinx to generate html documentation #708, ENH: Filter help text filled out. Docs updated with python section #673.
• R8. UnitTest::CheckArraysInheritTupleDims(dataStructure) missing from Fill Character Validation, Dimension Mismatch Validation, and 3D Image Single-File Output (present only in the templated test).
• R9. Strengthen the oracle. Add expected file count per plane (2 XY / 2 XZ / 3 YZ, and no extra files — Write Stack already does this via validateOutputFiles) and, once B2 is fixed, a spacing/origin round-trip assertion.
• R10. Report header placeholders. Verified commit and Sign-off are still <...>.
• R11. State the parametersVersion() decision. The fill-character rule changed from "≤ 1 character accepted" to "exactly 1", so a saved pipeline with an empty Fill Character now errors on load. The parameter set is unchanged, so leaving it at 2 is defensible, but per policy the decision should be recorded in the report with a one-line rationale.

Reproduction notes

All measurements above were taken from NX-Com-Qt69-Vtk96-Rel at PR head 4488f29, using temporary probe test cases that were removed afterward (working tree verified clean, 15/15 ITKImageWriter tests passing).