diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d09f18e8..554b1cfe6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,9 @@ and this project aspires to adhere to [Semantic Versioning](https://semver.org/s - Fixed missing implementation of DataType::is_index_t - Fixed issue with compiling t_h5z_zfp_smoke.cpp against an MPI-enabled HDF5. +#### Blueprint +- Fixed a bug that caused HDF5 reference paths to appear twice in Relay HDF5 Error messages. + #### Blueprint - `conduit::relay::io::blueprint.read_mesh` now uses read only I/O handles. diff --git a/src/libs/relay/conduit_relay_io_handle.cpp b/src/libs/relay/conduit_relay_io_handle.cpp index ff78b28e9..53d42d49b 100644 --- a/src/libs/relay/conduit_relay_io_handle.cpp +++ b/src/libs/relay/conduit_relay_io_handle.cpp @@ -711,7 +711,6 @@ void HDF5Handle::write(const Node &node, const Node &opts) { - CONDUIT_UNUSED(opts); // note: wrong mode errors are handled before dispatch to interface // Options Push / Pop (only needed for write, since hdf5 only supports @@ -723,7 +722,7 @@ HDF5Handle::write(const Node &node, hdf5_set_options(options()["hdf5"]); } - hdf5_write(node,m_h5_id); + hdf5_write(node,m_h5_id, opts); if(!prev_options.dtype().is_empty()) { diff --git a/src/libs/relay/conduit_relay_io_hdf5.cpp b/src/libs/relay/conduit_relay_io_hdf5.cpp index 266535cae..4f9be55a1 100644 --- a/src/libs/relay/conduit_relay_io_hdf5.cpp +++ b/src/libs/relay/conduit_relay_io_hdf5.cpp @@ -38,17 +38,17 @@ //----------------------------------------------------------------------------- #define CONDUIT_HDF5_ERROR( ref_path, msg ) \ { \ - CONDUIT_ERROR( "HDF5 Error (reference path: \"" << ref_path \ - << ref_path << "\") " << msg); \ + CONDUIT_ERROR( "HDF5 Error (reference path: \"" << ref_path << "\") " \ + << msg); \ } //----------------------------------------------------------------------------- /// The CONDUIT_HDF5_WARN macro is used for warnings with ref paths. //----------------------------------------------------------------------------- -#define CONDUIT_HDF5_WARN( ref_path, msg ) \ -{ \ - CONDUIT_WARN( "HDF5 Warning (reference path: \"" << ref_path \ - << ref_path << "\") " << msg); \ +#define CONDUIT_HDF5_WARN( ref_path, msg ) \ +{ \ + CONDUIT_WARN( "HDF5 Warning (reference path: \"" << ref_path << "\") " \ + << msg); \ } //----------------------------------------------------------------------------- @@ -1853,7 +1853,7 @@ setup_hdf5_group_atts_for_conduit_node(const Node &node, if( has_list_attr && node.dtype().is_object() ) { - std::cout << " remove_conduit_hdf5_list_attribute " << std::endl; + // std::cout << " remove_conduit_hdf5_list_attribute " << std::endl; remove_conduit_hdf5_list_attribute(hdf5_group_id, ref_path); } @@ -2256,9 +2256,12 @@ h5l_iterate_traverse_op_func(hid_t hdf5_id, << " parent: " << hdf5_id << " path:" << hdf5_path) ; - std::string chld_ref_path = h5_od->ref_path + - std::string("/") + - std::string(hdf5_path); + std::string chld_ref_path = h5_od->ref_path; + if(chld_ref_path != std::string("/")) + { + chld_ref_path += std::string("/"); + } + chld_ref_path += std::string(hdf5_path); switch (h5_info_buf.type) { @@ -2507,11 +2510,17 @@ read_hdf5_dataset_into_conduit_node(hid_t hdf5_dset_id, if (offset < 0) { - CONDUIT_ERROR("Offset must be non-negative."); + CONDUIT_HDF5_ERROR(ref_path, + "Error reading HDF5 Dataset with options:" + << opts.to_yaml() << + "`offset` must be non-negative."); } else if (stride < 1) { - CONDUIT_ERROR("Stride must be greater than zero."); + CONDUIT_HDF5_ERROR(ref_path, + "Error reading HDF5 Dataset with options:" + << opts.to_yaml() << + "`stride` must be greater than zero."); } // get the number of elements in the dataset given the offset and stride @@ -2527,7 +2536,10 @@ read_hdf5_dataset_into_conduit_node(hid_t hdf5_dset_id, nelems_to_read = opts["size"].to_value(); if (nelems_to_read < 1) { - CONDUIT_ERROR("Size must be greater than zero."); + CONDUIT_HDF5_ERROR(ref_path, + "Error reading HDF5 Dataset with options:" + << opts.to_yaml() << + "`size` must be greater than zero."); } } @@ -2614,7 +2626,11 @@ read_hdf5_dataset_into_conduit_node(hid_t hdf5_dset_id, else if( dt.number_of_elements() < 0 ) { CONDUIT_HDF5_ERROR(ref_path, - "Cannot read dataset with # of elements < 0"); + "Error reading HDF5 Dataset with options:" + << opts.to_yaml() + << "Cannot read using offset (" << offset << ")" + << " greater than the number of entries in" + << " the HDF5 dataset (" << nelems << ")"); } else if(dest.dtype().is_compact() && dest.dtype().compatible(dt) ) @@ -2651,11 +2667,26 @@ read_hdf5_dataset_into_conduit_node(hid_t hdf5_dset_id, H5Dclose(dataspace); } - CONDUIT_CHECK_HDF5_ERROR_WITH_FILE_AND_REF_PATH(h5_status, - hdf5_dset_id, - ref_path, - "Error reading HDF5 Dataset: " - << hdf5_dset_id); + if(opts.dtype().is_empty()) + { + CONDUIT_CHECK_HDF5_ERROR_WITH_FILE_AND_REF_PATH(h5_status, + hdf5_dset_id, + ref_path, + "Error reading HDF5 Dataset: " + << hdf5_dset_id); + } + else + { + CONDUIT_CHECK_HDF5_ERROR_WITH_FILE_AND_REF_PATH(h5_status, + hdf5_dset_id, + ref_path, + "Error reading HDF5 Dataset: " + << hdf5_dset_id + << " with options: " + << opts.to_yaml() + << "HDF5 dataset size: " + << nelems); + } CONDUIT_CHECK_HDF5_ERROR_WITH_FILE_AND_REF_PATH(H5Tclose(h5_dtype_id), hdf5_dset_id, diff --git a/src/tests/relay/t_relay_io_handle.cpp b/src/tests/relay/t_relay_io_handle.cpp index c60aba63f..53c569e40 100644 --- a/src/tests/relay/t_relay_io_handle.cpp +++ b/src/tests/relay/t_relay_io_handle.cpp @@ -395,7 +395,7 @@ TEST(conduit_relay_io_handle, test_reuse_handle) h.open("tout_conduit_relay_io_handle_reopen_1.conduit_bin"); h.write(n); h.close(); - + h.open("tout_conduit_relay_io_handle_reopen_2.conduit_bin"); h.write(n); h.close(); @@ -529,6 +529,204 @@ TEST(conduit_relay_io_handle, test_hdf5_trunc) } +//----------------------------------------------------------------------------- +TEST(conduit_relay_io_handle, test_offset_and_stride) +{ + Node n_about; + io::about(n_about); + + // skip test if hdf5 isn't enabled + if(n_about["protocols/hdf5"].as_string() != "enabled") + return; + + std::string tfile_out = "tout_hdf5_io_handle_with_offset.hdf5"; + // remove files if they already exist + utils::remove_path_if_exists(tfile_out); + + Node n, n_read, n_check, opts, info; + n["data"]= { 0,1,2,3,4,5,6,7,8,9}; + + io::IOHandle h; + h.open(tfile_out); + h.write(n); + + h.read(n_read); + n_read.print(); + + n_check = n; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + // strided read + n_read.reset(); + opts.reset(); + opts["stride"] = 2; + h.read(n_read,opts); + n_read.print(); + + n_check.reset(); + n_check["data"] = {0,2,4,6,8}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + // offset write + n = {-1,-1,-1,-1,-1}; + opts.reset(); + opts["offset"] = 5; + h.write(n,"data",opts); + + n_read.reset(); + h.read(n_read); + n_read.print(); + + n_check.reset(); + n_check["data"] = {0,1,2,3,4,-1,-1,-1,-1,-1}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // read the first part of the seq + opts.reset(); + opts["size"] = 5; + n_read.reset(); + h.read("data",n_read,opts); + n_read.print(); + + n_check.reset(); + n_check = {0,1,2,3,4}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + // read the second part of the seq (-1's) + opts.reset(); + opts["offset"] = 5; + n_read.reset(); + h.read("data",n_read,opts); + n_read.print(); + + n_check.reset(); + n_check = {-1,-1,-1,-1,-1}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // strided write + n = {1,1,1,1,1}; + opts.reset(); + opts["stride"] = 2; + h.write(n,"data",opts); + + // strided +offset write + n = {2,2,2,2,2}; + opts.reset(); + opts["offset"] = 1; + opts["stride"] = 2; + h.write(n,"data",opts); + + n_read.reset(); + h.read(n_read); + n_read.print(); + + n_check.reset(); + n_check["data"] = {1, 2, 1, 2, 1, 2, 1, 2, 1, 2}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // read the 1's + opts.reset(); + opts["offset"] = 0; + opts["stride"] = 2; + n_read.reset(); + h.read("data",n_read,opts); + n_read.print(); + + n_check.reset(); + n_check = {1, 1, 1, 1, 1}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // read the 2's + opts.reset(); + opts["offset"] = 1; + opts["stride"] = 2; + n_read.reset(); + h.read("data",n_read,opts); + n_read.print(); + + + n_check.reset(); + n_check = {2, 2, 2, 2, 2}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // read subset of the 2's + opts.reset(); + opts["offset"] = 1; + opts["stride"] = 2; + opts["size"] = 2; + n_read.reset(); + h.read("data",n_read,opts); + n_read.print(); + + n_check.reset(); + n_check = {2, 2}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + // huge stride, this will only read the first entry + n_read.reset(); + opts.reset(); + opts["stride"] = 1000; + h.read(n_read,opts); + n_read.print(); + + n_check.reset(); + n_check["data"] = {1}; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + + // now some error conditions: + + // neg size + n_read.reset(); + opts.reset(); + opts["size"] = -100; + EXPECT_THROW(h.read(n_read,opts),conduit::Error); + + + // neg stride + n_read.reset(); + opts.reset(); + opts["stride"] = -1; + EXPECT_THROW(h.read(n_read,opts),conduit::Error); + + // neg offset + n_read.reset(); + opts.reset(); + opts["offset"] = -1; + EXPECT_THROW(h.read(n_read,opts),conduit::Error); + + // // huge size + n_read.reset(); + opts.reset(); + opts["size"] = 1000; + EXPECT_THROW(h.read(n_read,opts),conduit::Error); + + // huge offset + n_read.reset(); + opts.reset(); + opts["offset"] = 1000; + EXPECT_THROW(h.read(n_read,opts),conduit::Error); + +} + + + + //----------------------------------------------------------------------------- TEST(conduit_relay_io_handle, test_empty_path_as_root) { @@ -574,3 +772,65 @@ TEST(conduit_relay_io_handle, test_empty_path_as_root) EXPECT_FALSE(n.diff(n_read_2, info, 0.0)); EXPECT_FALSE(n.diff(n_read_2, info, 0.0)); } + + +//----------------------------------------------------------------------------- +TEST(conduit_relay_io_handle, test_ref_path_error_msg) +{ + // check that ref path only appears in the error message string once + + Node n_about; + io::about(n_about); + + // skip test if hdf5 isn't enabled + if(n_about["protocols/hdf5"].as_string() != "enabled") + return; + + std::string tfile_out = "tout_hdf5_io_handle_for_ref_path_error_msg.hdf5"; + // remove files if they already exist + utils::remove_path_if_exists(tfile_out); + + Node n, n_read, n_check, opts, info; + n["my/path/to/some/data"]= { 0,1,2,3,4,5,6,7,8,9}; + + io::IOHandle h; + h.open(tfile_out); + h.write(n); + + h.read(n_read); + n_read.print(); + + n_check = n; + // expect no diff + EXPECT_FALSE(n_read.diff(n_check,info)); + + // huge offset + n_read.reset(); + opts.reset(); + opts["offset"] = 1000; + try + { + h.read(n_read,opts); + } + catch(conduit::Error &e) + { + std::string msg = e.message(); + std::cout << "error message:" + << msg << std::endl; + int count = 0; + + std::string::size_type pos = 0; + std::string path = "my/path/to/some/data"; + + while ((pos = msg.find(path, pos )) != std::string::npos) + { + count++; + pos += path.length(); + } + + std::cout << "# of occurrences of path: " << count << std::endl; + // the path should only appear in the error message string once + EXPECT_EQ(count,1); + } + +} diff --git a/src/tests/relay/t_relay_io_hdf5.cpp b/src/tests/relay/t_relay_io_hdf5.cpp index 6c7a28fd4..3acbe8e00 100644 --- a/src/tests/relay/t_relay_io_hdf5.cpp +++ b/src/tests/relay/t_relay_io_hdf5.cpp @@ -1873,3 +1873,57 @@ TEST(conduit_relay_io_hdf5, conduit_hdf5_compat_with_empty) EXPECT_FALSE(n.diff(n_load,n_diff_info)); } + +//----------------------------------------------------------------------------- +TEST(conduit_relay_io_hdf5, test_ref_path_error_msg) +{ + // check that ref path only appears in the error message string once + + Node n_about; + io::about(n_about); + + // skip test if hdf5 isn't enabled + if(n_about["protocols/hdf5"].as_string() != "enabled") + return; + + std::string tfile_out = "tout_hdf5_io_for_ref_path_error_msg.hdf5"; + // remove files if they already exist + utils::remove_path_if_exists(tfile_out); + + Node n, n_read, n_check, opts, info; + n["my/path/to/some/data"]= { 0,1,2,3,4,5,6,7,8,9}; + + io::save(n,tfile_out, "hdf5"); + + // bad offset + opts.reset(); + opts["offset"] = 1000; + + try + { + io::load(tfile_out,"hdf5",opts,n_read); + } + catch(conduit::Error &e) + { + std::string msg = e.message(); + std::cout << "error message:" + << msg << std::endl; + int count = 0; + + std::string::size_type pos = 0; + std::string path = "my/path/to/some/data"; + + while ((pos = msg.find(path, pos )) != std::string::npos) + { + count++; + pos += path.length(); + } + + std::cout << "# of occurrences of path: " << count << std::endl; + + // the path should only appear in the error message string once + EXPECT_EQ(count,1); + } + +} +