Skip to content

Add VTKHDF output support for all structured mesh types (#3620)#3877

Open
Jarvis2001 wants to merge 18 commits intoopenmc-dev:developfrom
Jarvis2001:vtkhdf
Open

Add VTKHDF output support for all structured mesh types (#3620)#3877
Jarvis2001 wants to merge 18 commits intoopenmc-dev:developfrom
Jarvis2001:vtkhdf

Conversation

@Jarvis2001
Copy link
Copy Markdown
Contributor

@Jarvis2001 Jarvis2001 commented Mar 13, 2026

Description

PR #3252 added .vtkhdf output for UnstructuredMesh. This PR extends that support to all four structured mesh types — RegularMesh, RectilinearMesh, CylindricalMesh, and SphericalMesh — following the same pattern. The legacy ASCII .vtk path is unchanged.


Changes

StructuredMesh.write_data_to_vtk

  • volume_normalization default changed from True to None, with format-appropriate fallback: False for .vtkhdf and True for legacy ASCII .vtk. The previous hardcoded True default would silently call self.volumes on 1D/2D meshes, which raises RuntimeError since volumes are only defined for 3D meshes.

  • Fixed unconditional .T.ravel() in the ASCII path for 3D datasets. The transpose is now conditional: RegularMesh and RectilinearMesh store data in C (ijk) order that matches the VTK writer directly — no transpose is needed or correct. CylindricalMesh and SphericalMesh still require the transpose to produce Fortran (kji) order expected by the curvilinear VTK writer. The same condition applies to volumes during normalization.

RegularMesh._write_vtk_hdf5 (new method)

  • Writes StructuredGrid VTKHDF format with explicit Points, Dimensions, and CellData, consistent with the existing UnstructuredMesh VTKHDF writer.
  • Type attribute written as a fixed-length ASCII HDF5 string via h5py.string_dtype, matching the UnstructuredMesh pattern. Without this, h5py stores a variable-length string that reads back as str rather than bytes, breaking == b"StructuredGrid" comparisons in VTK readers.
  • Dimensions carries only ndim entries (not padded to 3), so 1D and 2D meshes write the correct number of dimensions.
  • Guards against datasets=None, calls _reshape_vtk_dataset before validation, and validates both shape and element count.

RectilinearMesh._write_vtk_hdf5, CylindricalMesh._write_vtk_hdf5, SphericalMesh._write_vtk_hdf5 (new methods)

  • Same structure as RegularMesh._write_vtk_hdf5. Points are computed from the respective coordinate grids: Cartesian for RectilinearMesh, converted from (r, φ, z) for CylindricalMesh, and from (r, θ, φ) for SphericalMesh, each respecting the mesh origin.
  • Note: vtkRectilinearGrid is not part of the VTKHDF spec, so RectilinearMesh uses StructuredGrid as the closest equivalent.

New tests (test_mesh.py)

  • test_write_vtkhdf_regular_mesh — checks StructuredGrid type, Points, Dimensions, and CellData structure.
  • test_write_vtkhdf_rectilinear_mesh — checks file is created and CellData is populated.
  • test_write_vtkhdf_cylindrical_mesh — checks vertex Dimensions match (nr+1, nφ+1, nz+1).
  • test_write_vtkhdf_spherical_mesh — checks Points and CellData are present.
  • test_write_vtkhdf_volume_normalization — verifies both normalised and unnormalised output against known cell volumes.
  • test_write_vtkhdf_multiple_datasets — verifies multiple named datasets are written with correct data ordering (data.T.ravel()).
  • test_write_vtkhdf_invalid_data_shape — verifies ValueError is raised for shape mismatches.
  • test_write_vtkhdf_1d_mesh — verifies a 1D RegularMesh writes successfully without hitting the 3D-only volumes guard.
  • test_write_vtkhdf_2d_mesh — verifies a 2D RegularMesh writes the correct number of Dimensions entries.
  • test_write_ascii_vtk_unchanged — round-trip test confirming the legacy .vtk path is unaffected by these changes.

Fixes #3620


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)

@Jarvis2001 Jarvis2001 marked this pull request as draft March 13, 2026 17:39
@Jarvis2001 Jarvis2001 force-pushed the vtkhdf branch 2 times, most recently from 391fb53 to ede21b4 Compare March 13, 2026 19:42
@Jarvis2001 Jarvis2001 marked this pull request as ready for review March 13, 2026 21:31
@shimwell
Copy link
Copy Markdown
Member

Thanks for the PR i am keen to see vtkhdf export as an option for these meshes, thanks for your efforts in adding this feature.

My main question is about the code for sort each element's materials by material ID, is that something we need for the vtkhdf?

It is a bit hard to review this PR as there are a lot of changes here that are unrelated to the objective of the PR, mainly formatting changes. While these are good it does swamp the actual code additions of the PR a bit.

@Jarvis2001 Jarvis2001 marked this pull request as draft March 20, 2026 23:08
@Jarvis2001
Copy link
Copy Markdown
Contributor Author

Jarvis2001 commented Mar 21, 2026

Yeah, the material volume sorting is unrelated to the VTKHDF changes.
The test test_mesh_material_volumes_boundary_conditions was failing locally with:

        for evaluated, expected in zip(volumes.by_element(0), expected_volumes):
>           assert evaluated[0] == expected[0]
E           assert np.int32(24) == 23

../tests/unit_tests/test_mesh.py:716: AssertionError

It was a material ID ordering mismatch; the by_element() results came back in a different order than the test expected. The C library's ray traversal order isn't guaranteed, so whichever material a ray happens to hit first ends up first in the table. The sorting was added as a quick fix to make the test pass, but it was bundled into this PR. I have dropped it from this PR. Also, the reformatting was due to autopep8. I have reverted it. Thank you for pointing these things out.

@Jarvis2001 Jarvis2001 marked this pull request as ready for review March 21, 2026 07:00
@shimwell
Copy link
Copy Markdown
Member

shimwell commented Apr 2, 2026

I removed the duplicate tests and pushed to the branch, changed the type attribute, reverted some unrelated formatting changes (which I think are good, I am just trying to keep the PR minimal and on topic), So this addresses most of my earlier suggestions.

A potential bugs that I just spotted is that the order used for RectilinearMesh/Cylindrical/Spherical. The PR uses .reshape(-1, 3) (C-order) which needs checking as I think this is not right.

This needs checking up I think for those three meshes it would need changing from points = vertices.reshape(-1, 3) to points = np.swapaxes(vertices, 0, 2).reshape(-1, 3) to match the vtk writer.

@shimwell
Copy link
Copy Markdown
Member

shimwell commented Apr 2, 2026

It also looks like the RectilinearMesh vtkhdf writing only supports 3D meshes where the legacy vtk route supports both 1D/2D and 3D

@shimwell
Copy link
Copy Markdown
Member

shimwell commented Apr 2, 2026

The changes to volume_normalization=None also change the default behavior so I think this needs careful consideration.

I've done a decent amount of changes on this PR, let me know if you are happy with the direction of the changes.

@Jarvis2001
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review and changes. These were very useful.

I've addressed these:

It also looks like the RectilinearMesh vtkhdf writing only supports 3D meshes where the legacy vtk route supports both 1D/2D and 3D

The changes to volume_normalization=None also change the default behavior so I think this needs careful consideration.

In the latest commit.

  • RectilinearMesh now supports 1D/2D/3D data in VTKHDF (singleton dimension padding).
  • volume_normalization=None path is now explicitly tested (VTKHDF default behaviour is unnormalized).
  • Added dedicated tests for each area.

I've done a decent amount of changes on this PR, let me know if you are happy with the direction of the changes.

I’m happy with the direction of all the changes. Thank you.

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.

adding hdfvtk format export for all meshes

2 participants