From bc814d36d693252e262a88a250d743bd45595515 Mon Sep 17 00:00:00 2001 From: GuySten Date: Sun, 30 Aug 2026 21:56:59 +0300 Subject: [PATCH 1/2] fix --- include/openmc/hdf5_interface.h | 19 +- src/cell.cpp | 4 +- src/hdf5_interface.cpp | 44 ++-- src/lattice.cpp | 4 +- src/surface.cpp | 4 +- tests/cpp_unit_tests/CMakeLists.txt | 1 + tests/cpp_unit_tests/test_hdf5_interface.cpp | 213 +++++++++++++++++++ tests/unit_tests/test_hdf5_empty_strings.py | 116 ++++++++++ 8 files changed, 372 insertions(+), 33 deletions(-) create mode 100644 tests/cpp_unit_tests/test_hdf5_interface.cpp create mode 100644 tests/unit_tests/test_hdf5_empty_strings.py diff --git a/include/openmc/hdf5_interface.h b/include/openmc/hdf5_interface.h index 93e4bfc820a..b8532182b7f 100644 --- a/include/openmc/hdf5_interface.h +++ b/include/openmc/hdf5_interface.h @@ -1,13 +1,14 @@ #ifndef OPENMC_HDF5_INTERFACE_H #define OPENMC_HDF5_INTERFACE_H -#include // for min +#include // for min, max, find #include #include #include // for strlen #include #include #include +#include #include "hdf5.h" #include "hdf5_hl.h" @@ -185,12 +186,14 @@ inline void read_attribute(hid_t obj_id, const char* name, std::string& str) { // Create buffer to read data into auto n = attribute_typesize(obj_id, name); - char* buffer = new char[n]; + std::vector buffer(n, '\0'); // Read attribute and set string - read_attr_string(obj_id, name, n, buffer); - str = std::string {buffer, n}; - delete[] buffer; + read_attr_string(obj_id, name, n, buffer.data()); + + // As in read_dataset, the string ends at the first null character + auto end = std::find(buffer.begin(), buffer.end(), '\0'); + str = std::string {buffer.begin(), end}; } // overload for vector @@ -247,7 +250,11 @@ inline void read_dataset( // Read attribute and set string read_string(obj_id, name, n, buffer.data(), indep); - str = std::string {buffer.begin(), buffer.end()}; + + // Fixed-length strings are null-padded, and an empty string is stored as a + // single null byte, so the string ends at the first null character + auto end = std::find(buffer.begin(), buffer.end(), '\0'); + str = std::string {buffer.begin(), end}; } // array version diff --git a/src/cell.cpp b/src/cell.cpp index c9d6ecfcd5a..69f6586cde1 100644 --- a/src/cell.cpp +++ b/src/cell.cpp @@ -285,9 +285,7 @@ void Cell::to_hdf5(hid_t cell_group) const // Create a group for this cell. auto group = create_group(cell_group, fmt::format("cell {}", id_)); - if (!name_.empty()) { - write_string(group, "name", name_, false); - } + write_string(group, "name", name_, false); write_dataset(group, "universe", model::universes[universe_]->id_); diff --git a/src/hdf5_interface.cpp b/src/hdf5_interface.cpp index 00c6a4399c0..258711dc736 100644 --- a/src/hdf5_interface.cpp +++ b/src/hdf5_interface.cpp @@ -1,5 +1,6 @@ #include "openmc/hdf5_interface.h" +#include // for max #include #include #include @@ -587,17 +588,18 @@ void write_attr_int(hid_t obj_id, int ndim, const hsize_t* dims, void write_attr_string(hid_t obj_id, const char* name, const char* buffer) { - size_t n = strlen(buffer); - if (n > 0) { - // Set up appropriate datatype for a fixed-length string - hid_t datatype = H5Tcopy(H5T_C_S1); - H5Tset_size(datatype, n); + // As in write_string, an empty string is stored as a single null byte so that + // the attribute is always present in the file + size_t n = std::max(strlen(buffer), static_cast(1)); - write_attr(obj_id, 0, nullptr, name, datatype, buffer); + // Set up appropriate datatype for a fixed-length string + hid_t datatype = H5Tcopy(H5T_C_S1); + H5Tset_size(datatype, n); - // Free resources - H5Tclose(datatype); - } + write_attr(obj_id, 0, nullptr, name, datatype, buffer); + + // Free resources + H5Tclose(datatype); } void write_dataset_lowlevel(hid_t group_id, int ndim, const hsize_t* dims, @@ -662,17 +664,23 @@ void write_llong(hid_t group_id, int ndim, const hsize_t* dims, void write_string(hid_t group_id, int ndim, const hsize_t* dims, size_t slen, const char* name, const char* buffer, bool indep) { - if (slen > 0) { - // Set up appropriate datatype for a fixed-length string - hid_t datatype = H5Tcopy(H5T_C_S1); - H5Tset_size(datatype, slen); + // HDF5 has no zero-size string datatype, so an empty string is stored as a + // single null byte and stripped back off when read. Skipping the write + // entirely would leave the dataset absent from the file, which every reader + // that expects it then has to guard against. + const char null_char = '\0'; + const char* data = (slen > 0) ? buffer : &null_char; + size_t size = std::max(slen, static_cast(1)); - write_dataset_lowlevel( - group_id, ndim, dims, name, datatype, H5S_ALL, indep, buffer); + // Set up appropriate datatype for a fixed-length string + hid_t datatype = H5Tcopy(H5T_C_S1); + H5Tset_size(datatype, size); - // Free resources - H5Tclose(datatype); - } + write_dataset_lowlevel( + group_id, ndim, dims, name, datatype, H5S_ALL, indep, data); + + // Free resources + H5Tclose(datatype); } void write_string( diff --git a/src/lattice.cpp b/src/lattice.cpp index 9e56c58e8ec..c3954fe0b23 100644 --- a/src/lattice.cpp +++ b/src/lattice.cpp @@ -139,9 +139,7 @@ void Lattice::to_hdf5(hid_t lattices_group) const hid_t lat_group = create_group(lattices_group, group_name); // Write the name and outer universe. - if (!name_.empty()) { - write_string(lat_group, "name", name_, false); - } + write_string(lat_group, "name", name_, false); if (outer_ != NO_OUTER_UNIVERSE) { int32_t outer_id = model::universes[outer_]->id_; diff --git a/src/surface.cpp b/src/surface.cpp index 81b756deae7..6b8c82ed3a1 100644 --- a/src/surface.cpp +++ b/src/surface.cpp @@ -191,9 +191,7 @@ void Surface::to_hdf5(hid_t group_id) const } } - if (!name_.empty()) { - write_string(surf_group, "name", name_, false); - } + write_string(surf_group, "name", name_, false); to_hdf5_inner(surf_group); diff --git a/tests/cpp_unit_tests/CMakeLists.txt b/tests/cpp_unit_tests/CMakeLists.txt index 991f219f528..a3e2cca2d2c 100644 --- a/tests/cpp_unit_tests/CMakeLists.txt +++ b/tests/cpp_unit_tests/CMakeLists.txt @@ -1,6 +1,7 @@ set(TEST_NAMES test_distribution test_file_utils + test_hdf5_interface test_tally test_interpolate test_math diff --git a/tests/cpp_unit_tests/test_hdf5_interface.cpp b/tests/cpp_unit_tests/test_hdf5_interface.cpp new file mode 100644 index 00000000000..deb0bcf4440 --- /dev/null +++ b/tests/cpp_unit_tests/test_hdf5_interface.cpp @@ -0,0 +1,213 @@ +#include + +#include +#include + +#include "openmc/hdf5_interface.h" +#include "openmc/vector.h" + +using namespace openmc; + +namespace { + +//! Scoped HDF5 file that is created on construction and deleted on destruction +class TempFile { +public: + explicit TempFile(const std::string& filename) : filename_(filename) + { + file_id_ = file_open(filename_, 'w'); + } + + ~TempFile() + { + if (file_id_ >= 0) + file_close(file_id_); + std::remove(filename_.c_str()); + } + + //! Close the file and reopen it for reading, so that tests exercise what + //! actually landed on disk rather than a cached in-memory value + hid_t reopen() + { + file_close(file_id_); + file_id_ = file_open(filename_, 'r'); + return file_id_; + } + + hid_t id() const { return file_id_; } + +private: + std::string filename_; + hid_t file_id_; +}; + +} // namespace + +TEST_CASE("String datasets round-trip") +{ + TempFile file("test_hdf5_string_dataset.h5"); + + const std::string empty {""}; + const std::string nonempty {"pincell.exo"}; + // A string whose length is exactly the datatype size, i.e. with no room for a + // trailing null character. Reading must not truncate it. + const std::string exact {"abc"}; + + write_dataset(file.id(), "empty", empty); + write_dataset(file.id(), "nonempty", nonempty); + write_dataset(file.id(), "exact", exact); + write_dataset(file.id(), "empty_literal", ""); + + hid_t file_id = file.reopen(); + + SECTION("An empty string is still written as a dataset") + { + // Regression test for openmc-dev/openmc#2285: writing nothing at all left + // the dataset absent from the file and broke readers downstream + REQUIRE(object_exists(file_id, "empty")); + REQUIRE(object_exists(file_id, "empty_literal")); + + // Stored as a single null byte, since HDF5 has no zero-size string type + REQUIRE(dataset_typesize(file_id, "empty") == 1); + } + + SECTION("An empty string reads back as empty") + { + std::string value {"not empty"}; + read_dataset(file_id, "empty", value); + REQUIRE(value.empty()); + REQUIRE(value == ""); + + value = "not empty"; + read_dataset(file_id, "empty_literal", value); + REQUIRE(value.empty()); + } + + SECTION("A non-empty string is unchanged") + { + std::string value; + read_dataset(file_id, "nonempty", value); + REQUIRE(value == nonempty); + REQUIRE(dataset_typesize(file_id, "nonempty") == nonempty.size()); + } + + SECTION("A string with no room for a null terminator is not truncated") + { + std::string value; + read_dataset(file_id, "exact", value); + REQUIRE(value == exact); + REQUIRE(value.size() == 3); + } +} + +TEST_CASE("String datasets round-trip within a group") +{ + TempFile file("test_hdf5_string_group.h5"); + + hid_t group = create_group(file.id(), "geometry"); + write_dataset(group, "name", std::string {""}); + write_dataset(group, "region", std::string {"1 -2 3"}); + close_group(group); + + hid_t file_id = file.reopen(); + group = open_group(file_id, "geometry"); + + REQUIRE(object_exists(group, "name")); + + std::string name {"stale"}; + read_dataset(group, "name", name); + REQUIRE(name.empty()); + + std::string region; + read_dataset(group, "region", region); + REQUIRE(region == "1 -2 3"); + + close_group(group); +} + +TEST_CASE("String attributes round-trip") +{ + TempFile file("test_hdf5_string_attribute.h5"); + + write_attribute(file.id(), "empty", std::string {""}); + write_attribute(file.id(), "nonempty", std::string {"/path/to/inputs/"}); + write_attribute(file.id(), "empty_literal", ""); + write_attribute(file.id(), "exact", std::string {"abc"}); + + hid_t file_id = file.reopen(); + + SECTION("An empty attribute is still written") + { + // settings::path_input is empty unless -i is passed, so this is the common + // case for the "path" attribute of a statepoint file + REQUIRE(attribute_exists(file_id, "empty")); + REQUIRE(attribute_exists(file_id, "empty_literal")); + REQUIRE(attribute_typesize(file_id, "empty") == 1); + } + + SECTION("An empty attribute reads back as empty") + { + std::string value {"not empty"}; + read_attribute(file_id, "empty", value); + REQUIRE(value.empty()); + + value = "not empty"; + read_attribute(file_id, "empty_literal", value); + REQUIRE(value.empty()); + } + + SECTION("A non-empty attribute is unchanged") + { + std::string value; + read_attribute(file_id, "nonempty", value); + REQUIRE(value == "/path/to/inputs/"); + + read_attribute(file_id, "exact", value); + REQUIRE(value == "abc"); + } +} + +TEST_CASE("Vectors of strings round-trip") +{ + TempFile file("test_hdf5_string_vector.h5"); + + const vector mixed {"U235", "", "H1"}; + const vector all_empty {"", "", ""}; + const vector none {}; + + write_dataset(file.id(), "mixed", mixed); + write_dataset(file.id(), "all_empty", all_empty); + write_dataset(file.id(), "none", none); + + hid_t file_id = file.reopen(); + + SECTION("Individual empty entries do not shrink the datatype") + { + REQUIRE(object_exists(file_id, "mixed")); + // Sized by the longest entry plus a null terminator + REQUIRE(dataset_typesize(file_id, "mixed") == 5); + + hid_t dset = open_dataset(file_id, "mixed"); + REQUIRE(object_shape(dset)[0] == 3); + close_dataset(dset); + } + + SECTION("A vector of empty strings is written") + { + REQUIRE(object_exists(file_id, "all_empty")); + REQUIRE(dataset_typesize(file_id, "all_empty") == 1); + + hid_t dset = open_dataset(file_id, "all_empty"); + REQUIRE(object_shape(dset)[0] == 3); + close_dataset(dset); + } + + SECTION("An empty vector is written as a zero-length dataset") + { + REQUIRE(object_exists(file_id, "none")); + + hid_t dset = open_dataset(file_id, "none"); + REQUIRE(object_shape(dset)[0] == 0); + close_dataset(dset); + } +} diff --git a/tests/unit_tests/test_hdf5_empty_strings.py b/tests/unit_tests/test_hdf5_empty_strings.py new file mode 100644 index 00000000000..11c21aae8e9 --- /dev/null +++ b/tests/unit_tests/test_hdf5_empty_strings.py @@ -0,0 +1,116 @@ +"""Tests that the Python API reads empty strings written by OpenMC's C++ layer. + +An empty string has no zero-size representation in HDF5, so OpenMC stores it as +a single null byte. Older versions wrote nothing at all, leaving the dataset +absent from the file; both forms have to keep working. +""" + +import h5py +import numpy as np +import pytest + +import openmc + + +def write_string(group, name, value): + """Write a fixed-length string the way OpenMC's C++ layer does.""" + data = np.array(value.encode(), dtype=f'S{max(len(value), 1)}') + return group.create_dataset(name, data=data) + + +def write_string_attr(obj, name, value): + """Write a fixed-length string attribute the way OpenMC's C++ layer does.""" + data = np.array(value.encode(), dtype=f'S{max(len(value), 1)}') + obj.attrs.create(name, data) + + +@pytest.mark.parametrize('value', ['', 'a', 'some name', 'abc']) +def test_string_dataset_round_trip(run_in_tmpdir, value): + """The on-disk contract: what OpenMC writes is what h5py reads back.""" + with h5py.File('strings.h5', 'w') as f: + write_string(f, 'name', value) + + with h5py.File('strings.h5', 'r') as f: + assert 'name' in f + assert f['name'][()].decode() == value + + +@pytest.mark.parametrize('value', ['', 'a', '/path/to/inputs/']) +def test_string_attribute_round_trip(run_in_tmpdir, value): + with h5py.File('strings.h5', 'w') as f: + write_string_attr(f, 'path', value) + + with h5py.File('strings.h5', 'r') as f: + assert 'path' in f.attrs + assert f.attrs['path'].decode() == value + + +def test_string_vector_round_trip(run_in_tmpdir): + """Vectors of strings are null-padded to the longest entry.""" + names = ['U235', '', 'H1'] + with h5py.File('strings.h5', 'w') as f: + f.create_dataset('nuclides', data=np.array( + [n.encode() for n in names], dtype='S5')) + + with h5py.File('strings.h5', 'r') as f: + assert [n.decode() for n in f['nuclides'][()]] == names + + +# Each reader below is exercised twice: once against a file with the dataset +# present but empty (what OpenMC writes now) and once with it absent (what +# older versions wrote, and what the reader guards were added for). +@pytest.fixture(params=['present', 'absent']) +def empty_name(request): + """Return a function that writes an empty name, or doesn't.""" + def write(group): + if request.param == 'present': + write_string(group, 'name', '') + return write + + +def test_surface_from_hdf5_empty_name(run_in_tmpdir, empty_name): + with h5py.File('surfaces.h5', 'w') as f: + group = f.create_group('surface 1') + empty_name(group) + write_string(group, 'geom_type', 'csg') + write_string(group, 'type', 'x-plane') + write_string(group, 'boundary_type', 'vacuum') + group.create_dataset('coefficients', data=np.array([3.0])) + + surface = openmc.Surface.from_hdf5(group) + + assert surface.name == '' + assert surface.id == 1 + assert surface.boundary_type == 'vacuum' + assert surface.x0 == 3.0 + + +def test_material_from_hdf5_empty_name(run_in_tmpdir, empty_name): + with h5py.File('materials.h5', 'w') as f: + group = f.create_group('material 7') + empty_name(group) + group.attrs['depletable'] = 0 + group.create_dataset('atom_density', data=0.06022) + group.create_dataset('nuclides', data=np.array([b'H1'], dtype='S3')) + group.create_dataset('nuclide_densities', data=np.array([0.06022])) + + material = openmc.Material.from_hdf5(group) + + assert material.name == '' + assert material.id == 7 + + +def test_mesh_from_hdf5_empty_name(run_in_tmpdir, empty_name): + with h5py.File('meshes.h5', 'w') as f: + group = f.create_group('mesh 3') + empty_name(group) + write_string(group, 'type', 'regular') + group.create_dataset('dimension', data=np.array([2, 2, 2])) + group.create_dataset('lower_left', data=np.array([0.0, 0.0, 0.0])) + group.create_dataset('width', data=np.array([1.0, 1.0, 1.0])) + + mesh = openmc.MeshBase.from_hdf5(group) + + assert mesh.name == '' + assert mesh.id == 3 + assert mesh.dimension == (2, 2, 2) From 7e9951ea464c4736ef48f96e6e0024af93974c24 Mon Sep 17 00:00:00 2001 From: GuySten Date: Sun, 30 Aug 2026 22:27:41 +0300 Subject: [PATCH 2/2] increase minor version for statepoint --- include/openmc/constants.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/openmc/constants.h b/include/openmc/constants.h index a1d94e5819e..145d0329a0a 100644 --- a/include/openmc/constants.h +++ b/include/openmc/constants.h @@ -26,7 +26,7 @@ using double_4dvec = vector>>>; constexpr int HDF5_VERSION[] {3, 0}; // Version numbers for binary files -constexpr array VERSION_STATEPOINT {18, 2}; +constexpr array VERSION_STATEPOINT {18, 3}; constexpr array VERSION_PARTICLE_RESTART {2, 1}; constexpr array VERSION_TRACK {3, 1}; constexpr array VERSION_SUMMARY {6, 1};