From 0cd16e04e17c71b3edb15c23a4b9e015ebc403d0 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Tue, 20 Aug 2024 11:12:46 +0200 Subject: [PATCH] backport: Fix corner cases of combine_selection. (#1038, #1040) --- include/highfive/bits/H5Slice_traits.hpp | 29 +++++-- tests/unit/test_high_five_selection.cpp | 102 ++++++++++++++++++++--- 2 files changed, 111 insertions(+), 20 deletions(-) diff --git a/include/highfive/bits/H5Slice_traits.hpp b/include/highfive/bits/H5Slice_traits.hpp index a7231299c..9ece73b76 100644 --- a/include/highfive/bits/H5Slice_traits.hpp +++ b/include/highfive/bits/H5Slice_traits.hpp @@ -254,8 +254,25 @@ class HyperSlab { DataSpace combine_selections(const DataSpace& left_space, Op op, const DataSpace& right_space) const { - return detail::make_data_space( - H5Scombine_select(left_space.getId(), convert(op), right_space.getId())); + assert(op == Op::Or); + + auto left_type = detail::h5s_get_select_type(left_space.getId()); + auto right_type = detail::h5s_get_select_type(right_space.getId()); + + // Since HDF5 doesn't allow `combine_selections` with a None + // selection, we need to avoid the issue: + if (left_type == H5S_SEL_NONE) { + return right_space; + } else if (right_type == H5S_SEL_NONE) { + return left_space; + } else if (left_type == H5S_SEL_ALL) { + return left_space; + } else if (right_type == H5S_SEL_ALL) { + return right_space; + } else { + return detail::make_data_space( + detail::h5s_combine_select(left_space.getId(), convert(op), right_space.getId())); + } } /// Reduce a sequence of `Op::Or` efficiently. @@ -304,13 +321,7 @@ class HyperSlab { if (n_ors > 1) { auto right_space = reduce_streak(space_, begin, begin + n_ors, Op::Or); - // Since HDF5 doesn't allow `combine_selections` with a None - // selection, we need to avoid the issue: - if (detail::h5s_get_select_type(space.getId()) == H5S_SEL_NONE) { - space = right_space; - } else { - space = combine_selections(space, Op::Or, right_space); - } + space = combine_selections(space, Op::Or, right_space); i += n_ors - 1; } else if (selects[i].op == Op::None) { detail::h5s_select_none(space.getId()); diff --git a/tests/unit/test_high_five_selection.cpp b/tests/unit/test_high_five_selection.cpp index b082169d3..9dc3b6319 100644 --- a/tests/unit/test_high_five_selection.cpp +++ b/tests/unit/test_high_five_selection.cpp @@ -536,6 +536,17 @@ TEMPLATE_LIST_TEST_CASE("irregularHyperSlabSelectionWrite", "[template]", std::t irregularHyperSlabSelectionWriteTest(); } +void check_selected(const std::vector& selected, + const std::vector>& indices, + const std::vector>& x) { + REQUIRE(selected.size() == indices.size()); + for (size_t k = 0; k < selected.size(); ++k) { + size_t i = indices[k][0]; + size_t j = indices[k][1]; + REQUIRE(selected[k] == x[i][j]); + } +} + TEST_CASE("select_multiple_ors", "[hyperslab]") { size_t n = 100, m = 20; size_t nsel = 30; @@ -558,12 +569,7 @@ TEST_CASE("select_multiple_ors", "[hyperslab]") { SECTION("Pure Or Chain") { auto selected = dset.select(hyperslab).read>(); - REQUIRE(selected.size() == indices.size()); - for (size_t k = 0; k < selected.size(); ++k) { - size_t i = indices[k][0]; - size_t j = indices[k][1]; - REQUIRE(selected[k] == x[i][j]); - } + check_selected(selected, indices, x); } SECTION("Or Chain And Slab") { @@ -583,11 +589,85 @@ TEST_CASE("select_multiple_ors", "[hyperslab]") { hyperslab &= RegularHyperSlab(offsets, counts); auto selected = dset.select(hyperslab).read>(); - REQUIRE(selected.size() == selected_indices.size()); - for (size_t k = 0; k < selected.size(); ++k) { - size_t i = selected_indices[k][0]; - size_t j = selected_indices[k][1]; - REQUIRE(selected[k] == x[i][j]); + check_selected(selected, selected_indices, x); + } +} + +TEST_CASE("select_multiple_ors_edge_cases", "[hyperslab]") { + size_t n = 100, m = 20; + + auto x = testing::DataGenerator>>::create({n, m}); + auto file = File("select_multiple_ors_edge_cases.h5", File::Truncate); + auto dset = file.createDataSet("x", x); + + std::vector> indices; + for (size_t i = 0; i < n; ++i) { + for (size_t j = 0; j < m; ++j) { + indices.push_back({i, j}); } } + + auto space = DataSpace({n, m}); + SECTION("complete |= ") { + auto hyperslab = HyperSlab(RegularHyperSlab({0, 0}, {n, m})); + + // Select everything; and then redundantly some parts again. + hyperslab &= RegularHyperSlab({0, 0}, {n, m}); + hyperslab |= RegularHyperSlab({0, 0}, {n, m / 2}); + hyperslab |= RegularHyperSlab({3, 0}, {1, 3}); + hyperslab |= RegularHyperSlab({6, 0}, {1, 3}); + hyperslab.apply(space); + + auto selected = dset.select(hyperslab).read>(); + check_selected(selected, indices, x); + } + + SECTION("complete &=") { + auto hyperslab = HyperSlab(); + + // With detours, select everything, then reduce it to the first 2 + // elements. + hyperslab |= RegularHyperSlab({0, 0}, {n, m / 2}); + hyperslab |= RegularHyperSlab({0, 0}, {n, m / 2}); + hyperslab |= RegularHyperSlab({0, m / 2}, {n, m - m / 2}); + hyperslab |= RegularHyperSlab({0, 0}, {n, m}); + hyperslab &= RegularHyperSlab({0, 0}, {1, 2}); + hyperslab.apply(space); + + indices = {{0, 0}, {0, 1}}; + + auto selected = dset.select(hyperslab).read>(); + check_selected(selected, indices, x); + } + + SECTION("empty |=") { + auto hyperslab = HyperSlab(RegularHyperSlab({0, 0}, {n, m})); + hyperslab &= RegularHyperSlab({0, 0}, {1, 2}); + hyperslab &= RegularHyperSlab({3, 0}, {1, 2}); + + hyperslab |= RegularHyperSlab({0, 0}, {n, m / 2}); + hyperslab |= RegularHyperSlab({0, 0}, {n, m / 2}); + hyperslab |= RegularHyperSlab({0, m / 2}, {n, m - m / 2}); + hyperslab |= RegularHyperSlab({0, 0}, {n, m}); + + hyperslab.apply(space); + + auto selected = dset.select(hyperslab).read>(); + check_selected(selected, indices, x); + } + + SECTION("|= empty") { + auto hyperslab = HyperSlab(); + + hyperslab |= RegularHyperSlab({0, 0}, {1, 2}); + hyperslab |= RegularHyperSlab({0, 0}, {1, 2}); + hyperslab |= RegularHyperSlab({0, 0}, {0, 0}); + + hyperslab.apply(space); + + indices = {{0, 0}, {0, 1}}; + + auto selected = dset.select(hyperslab).read>(); + check_selected(selected, indices, x); + } }