From 78bc61d0a73771bd0e4b0e6a3478c0ff79d14db5 Mon Sep 17 00:00:00 2001 From: Nils Vu Date: Thu, 18 Apr 2024 15:28:41 -0700 Subject: [PATCH] Normalize subfile path in H5 lib This has bothered me for a long time. A leading slash and _no_ extension for subfile names was required before, and failing to do this lead to rather useless error messages. Now, the leading slash and the extension are handled within the H5 lib, so it should be much easier to use. --- src/IO/Exporter/Exporter.cpp | 13 ++----------- src/IO/Exporter/Exporter.hpp | 2 +- src/IO/H5/File.hpp | 13 +++++++++++-- src/IO/H5/Python/CombineH5.py | 5 ----- src/IO/H5/Python/ExtendConnectivityData.py | 5 ----- src/Visualization/Python/OpenVolfiles.py | 6 ------ src/Visualization/Python/Render1D.py | 5 ----- src/Visualization/Python/TransformVolumeData.py | 5 ----- tests/Unit/IO/H5/Test_Dat.cpp | 4 +++- tests/Unit/IO/H5/Test_VolumeData.cpp | 5 +++-- 10 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/IO/Exporter/Exporter.cpp b/src/IO/Exporter/Exporter.cpp index 571803de039b..b0862d4e7825 100644 --- a/src/IO/Exporter/Exporter.cpp +++ b/src/IO/Exporter/Exporter.cpp @@ -151,7 +151,7 @@ template std::vector> interpolate_to_points( const std::variant, std::string>& volume_files_or_glob, - std::string subfile_name, + const std::string& subfile_name, const std::variant& observation, const std::vector& tensor_components, const std::array, Dim>& target_points, @@ -173,15 +173,6 @@ std::vector> interpolate_to_points( ERROR_NO_TRACE("No volume files found. Specify at least one volume file."); } - // Normalize subfile name - if (subfile_name.size() > 4 && - subfile_name.substr(subfile_name.size() - 4) == ".vol") { - subfile_name = subfile_name.substr(0, subfile_name.size() - 4); - } - if (subfile_name.front() != '/') { - subfile_name = '/' + subfile_name; - } - // Retrieve info from the first volume file const h5::H5File first_h5file(filenames.front()); const auto& first_volfile = first_h5file.get(subfile_name); @@ -278,7 +269,7 @@ std::vector> interpolate_to_points( template std::vector> interpolate_to_points( \ const std::variant, std::string>& \ volume_files_or_glob, \ - std::string subfile_name, \ + const std::string& subfile_name, \ const std::variant& observation, \ const std::vector& tensor_components, \ const std::array, DIM(data)>& target_points, \ diff --git a/src/IO/Exporter/Exporter.hpp b/src/IO/Exporter/Exporter.hpp index 4e096c70dc6e..55778a856cf7 100644 --- a/src/IO/Exporter/Exporter.hpp +++ b/src/IO/Exporter/Exporter.hpp @@ -58,7 +58,7 @@ template std::vector> interpolate_to_points( const std::variant, std::string>& volume_files_or_glob, - std::string subfile_name, + const std::string& subfile_name, const std::variant& observation, const std::vector& tensor_components, const std::array, Dim>& target_points, diff --git a/src/IO/H5/File.hpp b/src/IO/H5/File.hpp index 7d3d5df244f8..1d14540f8eed 100644 --- a/src/IO/H5/File.hpp +++ b/src/IO/H5/File.hpp @@ -209,7 +209,7 @@ class H5File { template std::tuple check_if_object_exists( - const std::string& path) const; + std::string path) const; std::string file_name_; // NOLINTNEXTLINE(spectre-mutable) @@ -392,7 +392,16 @@ const ObjectType& H5File::convert_to_derived( template template std::tuple -H5File::check_if_object_exists(const std::string& path) const { +H5File::check_if_object_exists(std::string path) const { + // Normalize path to have a leading slash and the correct extension + if (path.front() != '/') { + path = '/' + path; + } + const size_t extension_length = ObjectType::extension().size(); + if (path.size() > extension_length and + path.substr(path.size() - extension_length) == ObjectType::extension()) { + path = path.substr(0, path.size() - extension_length); + } std::string name_only = "/"; if (path != "/") { name_only = file_system::get_file_name(path); diff --git a/src/IO/H5/Python/CombineH5.py b/src/IO/H5/Python/CombineH5.py index 4f5bad04cb93..a91d6ebdbf57 100644 --- a/src/IO/H5/Python/CombineH5.py +++ b/src/IO/H5/Python/CombineH5.py @@ -78,11 +78,6 @@ def combine_h5_vol_command(h5files, subfile_name, output, check_src): rich.print(rich.columns.Columns(spectre_file.all_vol_files())) return - if not subfile_name.startswith("/"): - subfile_name = "/" + subfile_name - if subfile_name.endswith(".vol"): - subfile_name = subfile_name[:-4] - if not output.endswith(".h5"): output += ".h5" diff --git a/src/IO/H5/Python/ExtendConnectivityData.py b/src/IO/H5/Python/ExtendConnectivityData.py index 2f6302a21ba2..ddb14917382e 100644 --- a/src/IO/H5/Python/ExtendConnectivityData.py +++ b/src/IO/H5/Python/ExtendConnectivityData.py @@ -40,11 +40,6 @@ def extend_connectivity_data_command(filename, subfile_name): files, the combine-h5 executable must be run first. The extend-connectivity command can then be run on the newly generated HDF5 file. """ - # Ensure that the format of the subfile is correct - if subfile_name.endswith(".vol"): - subfile_name = subfile_name[:-4] - if subfile_name[0] != "/": - subfile_name = "/" + subfile_name # Read the h5 file and extract the volume file from it h5_file = spectre_h5.H5File(filename, "r+") vol_file = h5_file.get_vol(subfile_name) diff --git a/src/Visualization/Python/OpenVolfiles.py b/src/Visualization/Python/OpenVolfiles.py index 04c81810a730..918e6019c59f 100644 --- a/src/Visualization/Python/OpenVolfiles.py +++ b/src/Visualization/Python/OpenVolfiles.py @@ -168,12 +168,6 @@ def command( rich.print(rich.columns.Columns(available_subfiles)) return - # Normalize subfile name - if subfile_name.endswith(".vol"): - subfile_name = subfile_name[:-4] - if not subfile_name.startswith("/"): - subfile_name = "/" + subfile_name - # Print available observations/times and exit if list_times: import rich.columns diff --git a/src/Visualization/Python/Render1D.py b/src/Visualization/Python/Render1D.py index 0607853be210..d1fb3bbd5c46 100755 --- a/src/Visualization/Python/Render1D.py +++ b/src/Visualization/Python/Render1D.py @@ -310,11 +310,6 @@ def render_1d_command( rich.print(rich.columns.Columns(open_h5_files[0].all_vol_files())) return - if subfile_name.endswith(".vol"): - subfile_name = subfile_name[:-4] - if not subfile_name.startswith("/"): - subfile_name = "/" + subfile_name - volfiles = [h5file.get_vol(subfile_name) for h5file in open_h5_files] dim = volfiles[0].get_dimension() assert ( diff --git a/src/Visualization/Python/TransformVolumeData.py b/src/Visualization/Python/TransformVolumeData.py index d65349e5d763..f5108ee734d6 100644 --- a/src/Visualization/Python/TransformVolumeData.py +++ b/src/Visualization/Python/TransformVolumeData.py @@ -800,11 +800,6 @@ def shift_magnitude( rich.print(rich.columns.Columns()) return - if subfile_name.endswith(".vol"): - subfile_name = subfile_name[:-4] - if not subfile_name.startswith("/"): - subfile_name = "/" + subfile_name - volfiles = [h5file.get_vol(subfile_name) for h5file in open_h5_files] # Load kernels diff --git a/tests/Unit/IO/H5/Test_Dat.cpp b/tests/Unit/IO/H5/Test_Dat.cpp index 3390149dd00c..d3635f071bf0 100644 --- a/tests/Unit/IO/H5/Test_Dat.cpp +++ b/tests/Unit/IO/H5/Test_Dat.cpp @@ -321,7 +321,9 @@ void test_dat_read() { error_file.append(std::vector{0.33, 0.66, 0.77, 0.90}); } h5::H5File my_file(h5_file_name); - const auto& error_file = my_file.get("/L2_errors"); + // No leading slash should also find the subfile, and a ".dat" extension as + // well + const auto& error_file = my_file.get("L2_errors.dat"); CHECK(error_file.subfile_path() == "/L2_errors"); // Check version info is correctly retrieved from Dat file diff --git a/tests/Unit/IO/H5/Test_VolumeData.cpp b/tests/Unit/IO/H5/Test_VolumeData.cpp index c819cafda002..7d1345fdbe91 100644 --- a/tests/Unit/IO/H5/Test_VolumeData.cpp +++ b/tests/Unit/IO/H5/Test_VolumeData.cpp @@ -264,9 +264,10 @@ void test() { my_file.close_current_object(); } // Open the read volume file and check that the observation id and values are - // correct. + // correct. No leading slash should also find the subfile, and a ".vol" + // extension as well. const auto& volume_file = - my_file.get("/element_data", version_number); + my_file.get("element_data.vol", version_number); CHECK(volume_file.subfile_path() == "/element_data"); const auto read_observation_ids = volume_file.list_observation_ids(); // The observation IDs should be sorted by their observation value