Skip to content

New Random Ray VTK Plot Testing Strategy - #4204

Merged
paulromano merged 2 commits into
openmc-dev:developfrom
jtramm:rr_vtk_test
Oct 26, 2026
Merged

paulromano merged 2 commits into
openmc-dev:developfrom
jtramm:rr_vtk_test

Conversation

@jtramm

@jtramm jtramm commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

#4192 fixed the double normalization of the random ray VTK flux, with a unit test that checks the mean of 8 voxels against a source region tally to within 15%. That test has several gaps:

  • The biggest issue is that it is fundamentally problematic to compare the VTK fluxes and the flux tallies at low batch/ray counts, as is typical for fast regression tests. The flux tallies are normalized by volume at every batch so as to use the same tallying pipeline that Monte Carlo does in OpenMC. The VTK fluxes are normalized once at the end of the simulation. The VTK route is technically better as the normal batch tally normalization does introduce a biased ratio estimator, which normalizing once at the end avoids. If you're just running a few batches with a few rays, that bias can be quite large, so you actually expect to see a significant difference between the VTK and the flux tallies. The two differ by up to 4.9% on the three-region cube at 5 inactive + 5 active batches. I'm guessing this is why the +/- 15% tolerance was added to the test originally. Generally though there's no limit here on how far apart the numbers might be for short tests, so if we wanted to make more tests following this pattern (e.g., to test linear sources, or different geometry types etc), we might run into more trouble.
  • If we added a linear source test (which would be good to do as there is special logic for plotting linear sources), it also wouldn't match the tallies necessarily as the plotted points from linear sources actually incorporate the gradient info, so we can visualize the gradients across large source regions in the plots. Conversely, we only receive the 0th order flux in each cell of a mesh flux tally. While careful averaging of all VTK points across a full cell should result in getting the average 0th order flux right, if there is a bit of an alignment issue with the points you might end up with more points closer to one side than another so you'd get a different 0th order term. Anyway, it's not going to be reliable.
  • It always runs serially, so the CI MPI job never covers the domain decomposed VTK writer.
  • For this particular test, the voxels sit at the centers of the source regions, where the linear source term is close to zero, so it still passes with that term removed or scaled by 100.

To remedy the above issues, this PR replaces the existing unit test with a more traditional "expected results" reference file regression test along with a new harness that facilitates reading/testing of the VTK files. This PR adds:

  • VTKTestHarness (tests/testing_harness.py) extends TolerantPyAPITestHarness. It keeps the voxel plot it is given (the base class removes all plots), appends the flux and source arrays of each VTK file to the results (adjoint runs also write a .forward.vtk), and deletes the VTK files on cleanup. The other arrays are not compared: FSRs and rank_subdomains are random colors, measured_load is timing dependent, and Materials and weight_window_lower are not the quantities under test. The harness is listed with the others in AGENTS.md.
  • tests/regression_tests/random_ray_vtk/ uses it for eigenvalue (random_ray_lattice) and fixed source (random_ray_three_region_cube) problems with flat and linear sources, with voxel centers away from region boundaries. The runs are 10 inactive + 5 active batches, so that every active batch lies past the ten batches in which the linear source keeps its gradients at zero. The four cases take about a second on 2 threads.

Adding more VTK tests in the future should be easy to do with the new harness. Also note that because the new test compares against stored reference results, it lives in tests/regression_tests/ rather than the unit suite, and the unit test is removed.

Follow-up to #4192.

AI Assistance

  • Harness: Claude Code
  • Model: Claude Opus 5.5 and Claude Fable 5.1
  • Reasoning effort: max

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 18) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

Summary by CodeRabbit

  • Tests

    • Added regression coverage for random-ray VTK output across eigenvalue and fixed-source modes, including flat and linear source shapes.
    • Enhanced test validation to compare voxel plots, flux arrays, and source arrays from generated VTK files.
    • Removed the previous standalone random-ray VTK normalization test in favor of integrated regression coverage.
  • Documentation

    • Documented the VTK regression-test harness among the available testing utilities.

John Tramm and others added 2 commits October 21, 2026 14:35
Add VTKTestHarness, a TolerantPyAPITestHarness that keeps one voxel plot
and appends the flux and source arrays of the VTK files it writes to the
results, and a random_ray_vtk regression test that uses it for
eigenvalue and fixed source problems with flat and linear sources.

This replaces test_random_ray_vtk_normalization from openmc-dev#4192, which
compared the mean of 8 voxels to a source region tally within 15%. Its
voxels sit at the centers of the source regions, where the linear term
is close to zero, so it still passes with that term removed, and it
always runs serially, so it never covers the domain decomposed writer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The linear source keeps its gradients at zero for the first ten batches,
so with the examples' 5 inactive + 5 active batches the linear cases
compared the moments of a flat source solve. Run 10 inactive + 5 active
batches instead, so that every active batch uses the gradients, and
regenerate the reference results.

Also say why the other VTK arrays are not compared, and list the harness
in AGENTS.md.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c69aec87-93cf-4678-97b6-8f6a12a93f36
📥 Commits

Reviewing files that changed from the base of the PR and between c630113 and 0f159ca.

⛔ Files ignored due to path filters: 8
  • tests/regression_tests/random_ray_vtk/eigen_flat/inputs_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/eigen_flat/results_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/eigen_linear/inputs_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/eigen_linear/results_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/fs_flat/inputs_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/fs_flat/results_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/fs_linear/inputs_true.dat is excluded by !**/*.dat
  • tests/regression_tests/random_ray_vtk/fs_linear/results_true.dat is excluded by !**/*.dat
📒 Files selected for processing: 5
  • AGENTS.md
  • tests/regression_tests/random_ray_vtk/__init__.py
  • tests/regression_tests/random_ray_vtk/test.py
  • tests/testing_harness.py
  • tests/unit_tests/test_random_ray_vtk_normalization.py
💤 Files with no reviewable changes: 1
  • tests/unit_tests/test_random_ray_vtk_normalization.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The change adds a VTK-aware regression test harness and parametrized random-ray VTK tests. It also documents the harness and removes the previous random-ray VTK normalization unit test.

Changes: Random-ray VTK regression coverage

Layer / File(s) Summary
Parse and collect VTK results
tests/testing_harness.py
read_vtk_arrays reads scalar arrays from legacy binary VTK files. VTKTestHarness adds selected flux and source arrays to test results, restores the supplied plot, and removes VTK outputs during cleanup.
Run random-ray VTK regression cases
tests/regression_tests/random_ray_vtk/test.py, AGENTS.md, tests/unit_tests/test_random_ray_vtk_normalization.py
Adds cases for eigen and fs run modes and flat and linear source shapes. The test harness cleanup removes mgxs.h5. The harness documentation is updated, and the previous normalization unit test is removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Suggested reviewers: paulromano, agourakis82

Merge Risk: ⚪ Minimal · up to 0f159

The new regression cases cover both VTK writer paths in CI. No change is needed before merging beyond normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: a new testing strategy for random-ray VTK plots.
Description check Passed The description provides the motivation, limitations of the replaced unit test, implementation details, test coverage, AI assistance disclosure, and completed checklist items. It references the relate…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description provides the motivation, limitations of the replaced unit test, implementation details, test coverage, AI assistance disclosure, and completed checklist items. It references the related issue and is sufficiently complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

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

Thanks for improving the robustness of these tests!

@paulromano
paulromano merged commit b1bb3ec into openmc-dev:develop Oct 26, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants