From a62a093e8ed7234b7c9453c62e6017b3dc779b77 Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Fri, 3 Nov 2023 09:23:07 +0100 Subject: [PATCH 1/7] Write currently failing test for ghost with deleted elements --- test/Makefile.am | 10 +++ test/t8_forest/t8_gtest_ghost_delete.cxx | 97 ++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 test/t8_forest/t8_gtest_ghost_delete.cxx diff --git a/test/Makefile.am b/test/Makefile.am index d7c1ec92f5..682c05c452 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -42,6 +42,7 @@ t8code_googletest_programs = \ test/t8_forest/t8_gtest_user_data \ test/t8_forest/t8_gtest_transform \ test/t8_forest/t8_gtest_ghost_exchange \ + test/t8_forest/t8_gtest_ghost_delete \ test/t8_forest/t8_gtest_ghost_and_owner \ test/t8_IO/t8_gtest_vtk_reader \ test/t8_forest_incomplete/t8_gtest_permute_hole \ @@ -210,6 +211,10 @@ test_t8_forest_t8_gtest_ghost_exchange_SOURCES = \ test/t8_gtest_main.cxx \ test/t8_forest/t8_gtest_ghost_exchange.cxx +test_t8_forest_t8_gtest_ghost_delete_SOURCES = \ + test/t8_gtest_main.cxx \ + test/t8_forest/t8_gtest_ghost_delete.cxx + test_t8_forest_t8_gtest_ghost_and_owner_SOURCES = \ test/t8_gtest_main.cxx \ test/t8_forest/t8_gtest_ghost_and_owner.cxx @@ -399,6 +404,10 @@ test_t8_forest_t8_gtest_ghost_exchange_LDADD = $(t8_gtest_target_ld_add) test_t8_forest_t8_gtest_ghost_exchange_LDFLAGS = $(t8_gtest_target_ld_flags) test_t8_forest_t8_gtest_ghost_exchange_CPPFLAGS = $(t8_gtest_target_cpp_flags) +test_t8_forest_t8_gtest_ghost_delete_LDADD = $(t8_gtest_target_ld_add) +test_t8_forest_t8_gtest_ghost_delete_LDFLAGS = $(t8_gtest_target_ld_flags) +test_t8_forest_t8_gtest_ghost_delete_CPPFLAGS = $(t8_gtest_target_cpp_flags) + test_t8_forest_t8_gtest_ghost_and_owner_LDADD = $(t8_gtest_target_ld_add) test_t8_forest_t8_gtest_ghost_and_owner_LDFLAGS = $(t8_gtest_target_ld_flags) test_t8_forest_t8_gtest_ghost_and_owner_CPPFLAGS = $(t8_gtest_target_cpp_flags) @@ -469,6 +478,7 @@ test_t8_geometry_t8_gtest_point_inside_CPPFLAGS += $(t8_gtest_target_mpi_cpp_fla test_t8_forest_t8_gtest_user_data_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) test_t8_forest_t8_gtest_transform_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) test_t8_forest_t8_gtest_ghost_exchange_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) +test_t8_forest_t8_gtest_ghost_delete_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) test_t8_forest_t8_gtest_ghost_and_owner_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) test_t8_IO_t8_gtest_vtk_reader_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) test_t8_forest_incomplete_t8_gtest_permute_hole_CPPFLAGS += $(t8_gtest_target_mpi_cpp_flags) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx new file mode 100644 index 0000000000..2dbe3c351c --- /dev/null +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -0,0 +1,97 @@ +/* + This file is part of t8code. + t8code is a C library to manage a collection (a forest) of multiple + connected adaptive space-trees of general element classes in parallel. + + Copyright (C) 2015 the developers + + t8code is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + t8code is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with t8code; if not, write to the Free Software Foundation, Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +*/ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* This test + */ + +static int test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which_tree, t8_locidx_t lelement_id, + t8_eclass_scheme_c *ts, const int is_family, const int num_elements, t8_element_t *elements[]) +{ + double coordinates[3]; + double eps = 0.0001; + t8_forest_element_coordinate (forest_from, which_tree, elements[0], 0, coordinates); + if (fabs(coordinates[1]) < eps) return 1; // refine the lowest row + if (fabs(coordinates[1] - 0.25) < eps) return -2; // refine the lowest row + return 0; +} + +class forest_ghost_exchange_holes: public testing::Test { + protected: + void + SetUp () override + { + /* adjust communicator size */ + int size, rank, color, key; + + sc_MPI_Comm_size(sc_MPI_COMM_WORLD, &size); + sc_MPI_Comm_rank(sc_MPI_COMM_WORLD, &rank); + t8_debugf("size is %i\n", size); + + if (rank < 2){ + color = 0; + key = rank; + }else{ + color = sc_MPI_UNDEFINED; + key = -1; + } + + t8_debugf("Color: %i, Key: %i\n", color, key); + + sc_MPI_Comm_split(sc_MPI_COMM_WORLD, color, key, &comm); + + sc_MPI_Comm_size(comm, &size); + t8_debugf("New size is: %i", size); + + + scheme = t8_scheme_new_default_cxx (); + /* Construct a cmesh */ + cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); + } + void + TearDown () override + { + //cmesh and scheme are freed taken and freed by forest + sc_MPI_Comm_free(&comm); + } + sc_MPI_Comm comm; + t8_scheme_cxx_t *scheme; + t8_cmesh_t cmesh; +}; + +TEST_F (forest_ghost_exchange_holes, errorTest) +{ + int level = 1; + t8_forest_t forest = t8_forest_new_uniform (cmesh, scheme, level, 1, comm); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); + t8_forest_unref(&forest); +} From 2df5acc94369919d0ebf90f2e546c151b95ca447 Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Fri, 3 Nov 2023 10:29:25 +0100 Subject: [PATCH 2/7] Add safety for procs whose subcommunicator is empty --- test/t8_forest/t8_gtest_ghost_delete.cxx | 31 +++++++++++++++--------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index 2dbe3c351c..3c0e286552 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -68,19 +68,24 @@ class forest_ghost_exchange_holes: public testing::Test { sc_MPI_Comm_split(sc_MPI_COMM_WORLD, color, key, &comm); - sc_MPI_Comm_size(comm, &size); - t8_debugf("New size is: %i", size); - + if(comm != sc_MPI_COMM_NULL){ + sc_MPI_Comm_size(comm, &size); + t8_debugf("\nNew size is: %i\n\n", size); + scheme = t8_scheme_new_default_cxx (); + /* Construct a cmesh */ + cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); + }else{ + t8_debugf("\nThis proc has comm NULL \n\n"); + } - scheme = t8_scheme_new_default_cxx (); - /* Construct a cmesh */ - cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); } void TearDown () override { //cmesh and scheme are freed taken and freed by forest - sc_MPI_Comm_free(&comm); + if(comm != sc_MPI_COMM_NULL){ + sc_MPI_Comm_free(&comm); + } } sc_MPI_Comm comm; t8_scheme_cxx_t *scheme; @@ -89,9 +94,11 @@ class forest_ghost_exchange_holes: public testing::Test { TEST_F (forest_ghost_exchange_holes, errorTest) { - int level = 1; - t8_forest_t forest = t8_forest_new_uniform (cmesh, scheme, level, 1, comm); - forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); - forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); - t8_forest_unref(&forest); + if(comm != sc_MPI_COMM_NULL){ + int level = 1; + t8_forest_t forest = t8_forest_new_uniform (cmesh, scheme, level, 1, comm); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); + t8_forest_unref(&forest); + } } From 7eb0a906365d64945ae0e985f57de96226613aa7 Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Fri, 3 Nov 2023 10:35:22 +0100 Subject: [PATCH 3/7] Add barrier so that empty procs don't trigger success while other procs are still working --- test/t8_forest/t8_gtest_ghost_delete.cxx | 1 + 1 file changed, 1 insertion(+) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index 3c0e286552..eb4bf98e74 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -86,6 +86,7 @@ class forest_ghost_exchange_holes: public testing::Test { if(comm != sc_MPI_COMM_NULL){ sc_MPI_Comm_free(&comm); } + sc_MPI_Barrier(sc_MPI_COMM_WORLD); } sc_MPI_Comm comm; t8_scheme_cxx_t *scheme; From ead3c042e739f8fed0d20b4c556274a9e232b7b8 Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Fri, 3 Nov 2023 11:14:33 +0100 Subject: [PATCH 4/7] spellcheck and indent --- test/t8_forest/t8_gtest_ghost_delete.cxx | 72 +++++++++++++----------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index eb4bf98e74..e3fbf82d98 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -33,15 +33,18 @@ /* This test */ -static int test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which_tree, t8_locidx_t lelement_id, - t8_eclass_scheme_c *ts, const int is_family, const int num_elements, t8_element_t *elements[]) +static int +test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which_tree, t8_locidx_t lelement_id, + t8_eclass_scheme_c *ts, const int is_family, const int num_elements, t8_element_t *elements[]) { - double coordinates[3]; - double eps = 0.0001; - t8_forest_element_coordinate (forest_from, which_tree, elements[0], 0, coordinates); - if (fabs(coordinates[1]) < eps) return 1; // refine the lowest row - if (fabs(coordinates[1] - 0.25) < eps) return -2; // refine the lowest row - return 0; + double coordinates[3]; + double eps = 0.0001; + t8_forest_element_coordinate (forest_from, which_tree, elements[0], 0, coordinates); + if (fabs (coordinates[1]) < eps) + return 1; // refine the lowest row + if (fabs (coordinates[1] - 0.25) < eps) + return -2; // refine the lowest row + return 0; } class forest_ghost_exchange_holes: public testing::Test { @@ -52,41 +55,42 @@ class forest_ghost_exchange_holes: public testing::Test { /* adjust communicator size */ int size, rank, color, key; - sc_MPI_Comm_size(sc_MPI_COMM_WORLD, &size); - sc_MPI_Comm_rank(sc_MPI_COMM_WORLD, &rank); - t8_debugf("size is %i\n", size); + sc_MPI_Comm_size (sc_MPI_COMM_WORLD, &size); + sc_MPI_Comm_rank (sc_MPI_COMM_WORLD, &rank); + t8_debugf ("size is %i\n", size); - if (rank < 2){ - color = 0; - key = rank; - }else{ - color = sc_MPI_UNDEFINED; - key = -1; + if (rank < 2) { + color = 0; + key = rank; + } + else { + color = sc_MPI_UNDEFINED; + key = -1; } - t8_debugf("Color: %i, Key: %i\n", color, key); + t8_debugf ("Color: %i, Key: %i\n", color, key); - sc_MPI_Comm_split(sc_MPI_COMM_WORLD, color, key, &comm); + sc_MPI_Comm_split (sc_MPI_COMM_WORLD, color, key, &comm); - if(comm != sc_MPI_COMM_NULL){ - sc_MPI_Comm_size(comm, &size); - t8_debugf("\nNew size is: %i\n\n", size); - scheme = t8_scheme_new_default_cxx (); - /* Construct a cmesh */ - cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); - }else{ - t8_debugf("\nThis proc has comm NULL \n\n"); + if (comm != sc_MPI_COMM_NULL) { + sc_MPI_Comm_size (comm, &size); + t8_debugf ("\nNew size is: %i\n\n", size); + scheme = t8_scheme_new_default_cxx (); + /* Construct a cmesh */ + cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); + } + else { + t8_debugf ("\nThis proc has comm NULL \n\n"); } - } void TearDown () override { //cmesh and scheme are freed taken and freed by forest - if(comm != sc_MPI_COMM_NULL){ - sc_MPI_Comm_free(&comm); + if (comm != sc_MPI_COMM_NULL) { + sc_MPI_Comm_free (&comm); } - sc_MPI_Barrier(sc_MPI_COMM_WORLD); + sc_MPI_Barrier (sc_MPI_COMM_WORLD); } sc_MPI_Comm comm; t8_scheme_cxx_t *scheme; @@ -95,11 +99,11 @@ class forest_ghost_exchange_holes: public testing::Test { TEST_F (forest_ghost_exchange_holes, errorTest) { - if(comm != sc_MPI_COMM_NULL){ + if (comm != sc_MPI_COMM_NULL) { int level = 1; t8_forest_t forest = t8_forest_new_uniform (cmesh, scheme, level, 1, comm); forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); - forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); - t8_forest_unref(&forest); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 0, NULL); + t8_forest_unref (&forest); } } From a6ea02f3c741fdc50310948ff1892e9c5d16e18a Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Fri, 3 Nov 2023 12:19:53 +0100 Subject: [PATCH 5/7] Correct comment --- test/t8_forest/t8_gtest_ghost_delete.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index e3fbf82d98..f9f7ad3074 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -43,7 +43,7 @@ test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which if (fabs (coordinates[1]) < eps) return 1; // refine the lowest row if (fabs (coordinates[1] - 0.25) < eps) - return -2; // refine the lowest row + return -2; // delete the higher row in the lower quadrants return 0; } From 6be3f9ed6462b3eaa334e522f5e0d8b5a40880cc Mon Sep 17 00:00:00 2001 From: "Dreyer, Lukas" Date: Thu, 16 Nov 2023 12:40:48 +0100 Subject: [PATCH 6/7] apply review --- test/t8_forest/t8_gtest_ghost_delete.cxx | 43 +++++++++++++++--------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index f9f7ad3074..05f4e118ce 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -30,19 +30,25 @@ #include #include -/* This test +/* This test is executed on a subcommunicator of exactly 2 procs, because it demonstrates a configuration that is currently not working + * A partitioned square of uniform refinement level 1 is adapted once, where only the lower half is refined. + * Then the mesh is adapted again, where only the upper half of the lower elements is deleted. + * Now, no element actually has a face connection to an existing element on the other proc, but they send a message anyway. */ + +/* refine elements, whose y coordinate is 0, so that the lowest row is refined + * delete elements, whose y coordniate is 0.25, so that the second row is deleted + */ static int test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which_tree, t8_locidx_t lelement_id, t8_eclass_scheme_c *ts, const int is_family, const int num_elements, t8_element_t *elements[]) { double coordinates[3]; - double eps = 0.0001; t8_forest_element_coordinate (forest_from, which_tree, elements[0], 0, coordinates); - if (fabs (coordinates[1]) < eps) + if (fabs (coordinates[1]) < T8_PRECISION_EPS) return 1; // refine the lowest row - if (fabs (coordinates[1] - 0.25) < eps) + if (fabs (coordinates[1] - 0.25) < T8_PRECISION_EPS) return -2; // delete the higher row in the lower quadrants return 0; } @@ -52,12 +58,11 @@ class forest_ghost_exchange_holes: public testing::Test { void SetUp () override { - /* adjust communicator size */ + /* adjust communicator size, we split the comm here, because we know that the test fails on 2 procs*/ int size, rank, color, key; sc_MPI_Comm_size (sc_MPI_COMM_WORLD, &size); sc_MPI_Comm_rank (sc_MPI_COMM_WORLD, &rank); - t8_debugf ("size is %i\n", size); if (rank < 2) { color = 0; @@ -67,20 +72,17 @@ class forest_ghost_exchange_holes: public testing::Test { color = sc_MPI_UNDEFINED; key = -1; } - - t8_debugf ("Color: %i, Key: %i\n", color, key); - sc_MPI_Comm_split (sc_MPI_COMM_WORLD, color, key, &comm); + T8_ASSERT (rank < 2 || comm == sc_MPI_COMM_NULL); if (comm != sc_MPI_COMM_NULL) { sc_MPI_Comm_size (comm, &size); - t8_debugf ("\nNew size is: %i\n\n", size); + T8_ASSERT(size <= 2); scheme = t8_scheme_new_default_cxx (); /* Construct a cmesh */ cmesh = t8_cmesh_new_hypercube (T8_ECLASS_QUAD, comm, 0, 0, 0); - } - else { - t8_debugf ("\nThis proc has comm NULL \n\n"); + }else{ + T8_ASSERT(rank >= 2); } } void @@ -91,6 +93,8 @@ class forest_ghost_exchange_holes: public testing::Test { sc_MPI_Comm_free (&comm); } sc_MPI_Barrier (sc_MPI_COMM_WORLD); + t8_cmesh_unref (&cmesh); + t8_scheme_cxx_unref (&scheme); } sc_MPI_Comm comm; t8_scheme_cxx_t *scheme; @@ -99,11 +103,18 @@ class forest_ghost_exchange_holes: public testing::Test { TEST_F (forest_ghost_exchange_holes, errorTest) { + /* This test tests the functionality described in Issue: https://github.com/DLR-AMR/t8code/issues/825 + * Remove the GTEST_SKIP() macros when you start working on the issue. + */ + GTEST_SKIP(); if (comm != sc_MPI_COMM_NULL) { - int level = 1; + const int level = 1; + const int execute_ghost = 1; + t8_cmesh_ref(cmesh); + t8_scheme_cxx_ref(scheme); t8_forest_t forest = t8_forest_new_uniform (cmesh, scheme, level, 1, comm); - forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 1, NULL); - forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, 0, NULL); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, execute_ghost, NULL); + forest = t8_forest_new_adapt (forest, test_adapt_holes, 0, execute_ghost, NULL); t8_forest_unref (&forest); } } From 29e6d4db5192d1cb4da0eb10eb754787aee48c62 Mon Sep 17 00:00:00 2001 From: Johannes Holke Date: Fri, 24 Nov 2023 15:27:40 +0100 Subject: [PATCH 7/7] Improve code documentation --- test/t8_forest/t8_gtest_ghost_delete.cxx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/t8_forest/t8_gtest_ghost_delete.cxx b/test/t8_forest/t8_gtest_ghost_delete.cxx index 05f4e118ce..43d2983b59 100644 --- a/test/t8_forest/t8_gtest_ghost_delete.cxx +++ b/test/t8_forest/t8_gtest_ghost_delete.cxx @@ -30,15 +30,17 @@ #include #include -/* This test is executed on a subcommunicator of exactly 2 procs, because it demonstrates a configuration that is currently not working +/* This test is executed on a subcommunicator of exactly 2 procs, because it demonstrates a configuration that is currently not working. See https://github.com/DLR-AMR/t8code/issues/825. * A partitioned square of uniform refinement level 1 is adapted once, where only the lower half is refined. * Then the mesh is adapted again, where only the upper half of the lower elements is deleted. - * Now, no element actually has a face connection to an existing element on the other proc, but they send a message anyway. + * Now, no element actually has a face connection to an existing element on the other proc, thus the Ghost structure should be empty. + * Within the bug discussed in https://github.com/DLR-AMR/t8code/issues/825 they send a message anyway. + * Once, https://github.com/DLR-AMR/t8code/issues/825 is resolved, no messages should be send and the test should pass. */ -/* refine elements, whose y coordinate is 0, so that the lowest row is refined - * delete elements, whose y coordniate is 0.25, so that the second row is deleted +/* refine elements, whose lower left y coordinate is 0, so that the lowest row is refined + * delete elements, whose lower left y coordniate is 0.25, so that the second row is deleted */ static int test_adapt_holes (t8_forest_t forest, t8_forest_t forest_from, t8_locidx_t which_tree, t8_locidx_t lelement_id,