Introduce populate functionality according to cmesh borders #998

lukasdreyer · 2024-03-25T07:55:47Z

Describe your changes here:
Unclean version of creating a forest according to the partitioned cmesh boundaries (cmesh elements are not allowed to be shared).
For discussions with @CsatiZoltan about whether this satisfies his requirements.

All these boxes must be checked by the reviewers before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

General

The reviewer executed the new code features at least once and checked the results manually
The code follows the t8code coding guidelines
New source/header files are properly added to the Makefiles
The code is well documented
All function declarations, structs/classes and their members have a proper doxygen documentation
All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue)

Tests

The code is covered in an existing or new test case using Google Test

Github action

The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)
All tests pass (in various configurations, this should be executed automatically in a github action)

If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
- Should this use case be added to the github action?
- If not, does the specific use case compile and all tests pass (check manually)

Scripts and Wiki

If a new directory with source-files is added, it must be covered by the script/find_all_source_files.scp to check the indentation of these files.
If this PR introduces a new feature, it must be covered in an example/tutorial and a Wiki article.

Licence

The author added a BSD statement to doc/ (or already has one)

CsatiZoltan · 2024-03-25T13:49:41Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+  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


Maybe change the date to 2024?

Yes, I already have the template for VScode that creates the header for new files, but this file was still created via copy paste before, so I will change it

How do you create a template for these things in VS Code (I'm curious because I use that editor too)?

CsatiZoltan · 2024-03-25T16:23:53Z

Here are two additional tests with three quadratic quads. The two_one_quad should fail, theone_two_quad test should pass.

TEST (t8_forest_new_from_partition, one_two_quad)
{
  /* Current process */
  int rank, size;
  sc_MPI_Comm_size (sc_MPI_COMM_WORLD, &size);
  sc_MPI_Comm_rank (sc_MPI_COMM_WORLD, &rank);

  /* Geometry */
  int degree = 2;
  std::vector<std::vector<double>> cells;
  cells.push_back (std::vector<double> { -1, 1.2, 0,   0, 1.2,  0,   -1, 3,    0, 0, 3,    0,   -1, 2.1,
                                         0,  0,   2.1, 0, -0.5, 1.2, 0,  -0.5, 3, 0, -0.5, 2.1, 0 });
  cells.push_back (std::vector<double> { 0, 1.2, 0,   2, 1.5, 0,    0, 3, 0, 2, 3,   0,   0, 2.1,
                                         0, 1.7, 2.1, 0, 1,   1.55, 0, 1, 3, 0, 0.9, 2.1, 0 });
  cells.push_back (
    std::vector<double> { 2, 1.5, 0, 4, 0, 0, 2, 3, 0, 4, 3, 0, 1.7, 2.1, 0, 4, 1.5, 0, 3, 1, 0, 3, 3, 0, 3, 2, 0 });

  /* Create a partitioned cmesh */
  t8_cmesh_t cmesh;
  t8_cmesh_init (&cmesh);
  if (size != 2)
    SC_ABORT ("Program must be run on two MPI processes.\n");
  else {
    if (rank == 0) {
      t8_cmesh_set_tree_class (cmesh, 0, T8_ECLASS_QUAD);
      t8_cmesh_set_tree_class (cmesh, 1, T8_ECLASS_QUAD);
      t8_cmesh_set_tree_vertices (cmesh, 0, cells[0].data (), cells[0].size ());
      t8_cmesh_set_tree_vertices (cmesh, 1, cells[1].data (), cells[1].size ());
      t8_cmesh_set_attribute (cmesh, 0, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree, sizeof (int), 1);
      t8_cmesh_set_attribute (cmesh, 1, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree, sizeof (int), 1);
      t8_cmesh_set_join (cmesh, 0, 1, 1, 0, 0);
      t8_cmesh_set_partition_range (cmesh, 3, 0, 0);
    }
    else if (rank == 1) {
      for (size_t itree = 0; itree < 3; ++itree) {
        t8_cmesh_set_tree_class (cmesh, itree, T8_ECLASS_QUAD);
        t8_cmesh_set_tree_vertices (cmesh, itree, cells[itree].data (), cells[itree].size ());
        t8_cmesh_set_attribute (cmesh, itree, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree,
                                sizeof (int), 1);
      }
      t8_cmesh_set_join (cmesh, 0, 1, 1, 0, 0);
      t8_cmesh_set_join (cmesh, 1, 2, 1, 0, 0);
      t8_cmesh_set_partition_range (cmesh, 3, 1, 2);
    }
  }
  t8_cmesh_register_geometry<t8_geometry_lagrange> (cmesh, 2);
  t8_cmesh_commit (cmesh, sc_MPI_COMM_WORLD);
  t8_cmesh_vtk_write_file (cmesh, "cmesh");
  t8_forest_t forest
    = t8_forest_new_uniform_with_cmesh_partition (cmesh, t8_scheme_new_default_cxx (), 0, 0, sc_MPI_COMM_WORLD);
  // t8_forest_t forest = t8_forest_new_uniform (cmesh, t8_scheme_new_default_cxx (), 0, 1, sc_MPI_COMM_WORLD);
  t8_forest_write_vtk (forest, "forest");
  t8_cmesh_destroy (&cmesh);
}

TEST (t8_forest_new_from_partition, two_one_quad)
{
  /* Current process */
  int rank, size;
  sc_MPI_Comm_size (sc_MPI_COMM_WORLD, &size);
  sc_MPI_Comm_rank (sc_MPI_COMM_WORLD, &rank);

  /* Geometry */
  int degree = 2;
  std::vector<std::vector<double>> cells;
  cells.push_back (std::vector<double> { -1, 1.2, 0,   0, 1.2,  0,   -1, 3,    0, 0, 3,    0,   -1, 2.1,
                                         0,  0,   2.1, 0, -0.5, 1.2, 0,  -0.5, 3, 0, -0.5, 2.1, 0 });
  cells.push_back (std::vector<double> { 0, 1.2, 0,   2, 1.5, 0,    0, 3, 0, 2, 3,   0,   0, 2.1,
                                         0, 1.7, 2.1, 0, 1,   1.55, 0, 1, 3, 0, 0.9, 2.1, 0 });
  cells.push_back (
    std::vector<double> { 2, 1.5, 0, 4, 0, 0, 2, 3, 0, 4, 3, 0, 1.7, 2.1, 0, 4, 1.5, 0, 3, 1, 0, 3, 3, 0, 3, 2, 0 });

  /* Create a partitioned cmesh */
  t8_cmesh_t cmesh;
  t8_cmesh_init (&cmesh);
  if (size != 2)
    SC_ABORT ("Program must be run on two MPI processes.\n");
  else {
    if (rank == 0) {
      for (size_t itree = 0; itree < 3; ++itree) {
        t8_cmesh_set_tree_class (cmesh, itree, T8_ECLASS_QUAD);
        t8_cmesh_set_tree_vertices (cmesh, itree, cells[itree].data (), cells[itree].size ());
        t8_cmesh_set_attribute (cmesh, itree, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree,
                                sizeof (int), 1);
      }
      t8_cmesh_set_join (cmesh, 0, 1, 1, 0, 0);
      t8_cmesh_set_join (cmesh, 1, 2, 1, 0, 0);
      t8_cmesh_set_partition_range (cmesh, 3, 0, 1);
    }
    else if (rank == 1) {
      t8_cmesh_set_tree_class (cmesh, 1, T8_ECLASS_QUAD);
      t8_cmesh_set_tree_class (cmesh, 2, T8_ECLASS_QUAD);
      t8_cmesh_set_tree_vertices (cmesh, 1, cells[1].data (), cells[1].size ());
      t8_cmesh_set_tree_vertices (cmesh, 2, cells[2].data (), cells[2].size ());
      t8_cmesh_set_attribute (cmesh, 1, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree, sizeof (int), 1);
      t8_cmesh_set_attribute (cmesh, 2, t8_get_package_id (), T8_CMESH_LAGRANGE_POLY_DEGREE, &degree, sizeof (int), 1);
      t8_cmesh_set_join (cmesh, 1, 2, 1, 0, 0);
      t8_cmesh_set_partition_range (cmesh, 3, 2, 2);
    }
  }
  t8_cmesh_register_geometry<t8_geometry_lagrange> (cmesh, 2);
  t8_cmesh_commit (cmesh, sc_MPI_COMM_WORLD);
  t8_cmesh_vtk_write_file (cmesh, "cmesh");
  t8_forest_t forest
    = t8_forest_new_uniform_with_cmesh_partition (cmesh, t8_scheme_new_default_cxx (), 0, 0, sc_MPI_COMM_WORLD);
  // t8_forest_t forest = t8_forest_new_uniform (cmesh, t8_scheme_new_default_cxx (), 0, 1, sc_MPI_COMM_WORLD);
  t8_forest_write_vtk (forest, "forest");
  t8_cmesh_destroy (&cmesh);
}

Davknapp

A first batch of comments for your code.
I need to review the changes in the t8_forest_populate function, but I need a clear mind for that. Probably will do it tomorrow.

Davknapp · 2024-07-08T13:41:27Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+  /* create a line cmesh */
+  int mpirank, mpisize;
+  sc_MPI_Comm comm = sc_MPI_COMM_WORLD;
+  sc_MPI_Comm_size (comm, &mpisize);


please check the return values after calling this function.

Davknapp · 2024-07-08T13:41:40Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+  sc_MPI_Comm comm = sc_MPI_COMM_WORLD;
+  sc_MPI_Comm_size (comm, &mpisize);
+  sc_MPI_Comm_rank (comm, &mpirank);
+  int face_knowledge = 3;


Suggested change

int face_knowledge = 3;

const int face_knowledge = 3;

Davknapp · 2024-07-08T13:41:58Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+  sc_MPI_Comm_size (comm, &mpisize);
+  sc_MPI_Comm_rank (comm, &mpirank);
+  int face_knowledge = 3;
+  int num_trees = 2;


Suggested change

int num_trees = 2;

const t8_gloidx_t num_trees = 2;

Davknapp · 2024-07-08T13:44:29Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+    t8_cmesh_set_tree_class (cmesh, second, T8_ECLASS_LINE);
+    t8_cmesh_set_tree_vertices (cmesh, first, vertices, 2);
+    t8_cmesh_set_tree_vertices (cmesh, second, vertices + 3, 2);
+    //    t8_cmesh_set_join (cmesh, first, second, 1, 0, 0);


Suggested change

// t8_cmesh_set_join (cmesh, first, second, 1, 0, 0);

Davknapp · 2024-07-08T13:45:32Z

test/t8_forest/t8_gtest_forest_new_from_part_cmesh.cxx

+    t8_cmesh_set_partition_range (cmesh, face_knowledge, 0, 1);
+  }
+  else {
+    t8_cmesh_set_dimension (cmesh, 1);


The dimension is not set in the first case. It should be set in both cases, I think you can move it out of the if/else part

Davknapp · 2024-07-08T14:45:50Z

src/t8_forest/t8_forest_cxx.cxx

+      /* Allocate elements for this processor. */
+      t8_element_array_init_size (telements, eclass_scheme, end - start);
+      t8_element_t *element = t8_element_array_index_locidx_mutable (telements, 0);
+      eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);


Suggested change

eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);

eclass_scheme->t8_element_set_linear_id (element, forest->set_level, 0);

Davknapp · 2024-07-08T14:46:11Z

src/t8_forest/t8_forest_cxx.cxx

+      t8_element_t *element = t8_element_array_index_locidx_mutable (telements, 0);
+      eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);
+      count_elements++;
+      for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {


Suggested change

for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {

for (t8_locidx_t et = 1; et < end; et++, count_elements++) {

Davknapp · 2024-07-08T14:46:20Z

src/t8_forest/t8_forest_cxx.cxx

+      eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);
+      count_elements++;
+      for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {
+        t8_element_t *element_succ = t8_element_array_index_locidx_mutable (telements, et - start);


Suggested change

t8_element_t *element_succ = t8_element_array_index_locidx_mutable (telements, et - start);

t8_element_t *element_succ = t8_element_array_index_locidx_mutable (telements, et);

Davknapp · 2024-07-08T14:46:39Z

src/t8_forest/t8_forest_cxx.cxx

+      t8_element_t *element = t8_element_array_index_locidx_mutable (telements, 0);
+      eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);
+      count_elements++;
+      for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {


et is not a good name for a variable

Davknapp · 2024-07-08T14:47:48Z

src/t8_forest/t8_forest.c

    T8_ASSERT (forest->set_level <= forest->maxlevel);
    /* populate a new forest with tree and quadrant objects */
    if (t8_forest_refines_irregular (forest) && forest->set_level > 0) {
      /* On root level we will also use the normal algorithm */
-      t8_forest_populate_irregular (forest);
+      t8_forest_populate_irregular (forest, 1);
+      //      t8_forest_populate_irregular (forest, !forest->set_initial_partition_according_to_cmesh);


Suggested change

// t8_forest_populate_irregular (forest, !forest->set_initial_partition_according_to_cmesh);

TODO: investigate further, because it looks like the second argument is not even used in t8_forest_populate_irregular

Davknapp · 2024-07-09T14:03:52Z

src/t8_forest/t8_forest_cxx.cxx

-  /* True if the forest has no elements */
-  is_empty = forest->first_local_tree > forest->last_local_tree
-             || (forest->first_local_tree == forest->last_local_tree && child_in_tree_begin >= child_in_tree_end);
+  t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);


Suggested change

t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);

const t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);

Davknapp · 2024-07-09T14:04:02Z

src/t8_forest/t8_forest_cxx.cxx

-  is_empty = forest->first_local_tree > forest->last_local_tree
-             || (forest->first_local_tree == forest->last_local_tree && child_in_tree_begin >= child_in_tree_end);
+  t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);
+  t8_gloidx_t cmesh_last_tree = cmesh_first_tree + t8_cmesh_get_num_local_trees (forest->cmesh) - 1;


Suggested change

t8_gloidx_t cmesh_last_tree = cmesh_first_tree + t8_cmesh_get_num_local_trees (forest->cmesh) - 1;

const t8_gloidx_t cmesh_last_tree = cmesh_first_tree + t8_cmesh_get_num_local_trees (forest->cmesh) - 1;

Davknapp · 2024-07-09T14:04:31Z

src/t8_forest/t8_forest_cxx.cxx


-  if (!is_empty) {
+  /* True if the forest has no elements */
+  int is_empty


Suggested change

int is_empty

const bool is_empty

Davknapp · 2024-07-09T14:06:10Z

src/t8_forest/t8_forest_cxx.cxx

-      tree_class = tree->eclass = t8_cmesh_get_tree_class (forest->cmesh, jt - first_ctree);
+    t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);
+    t8_locidx_t count_elements = 0;
+    for (t8_gloidx_t jt = forest->first_local_tree; jt <= forest->last_local_tree; jt++) {


jt is not a good name for a variable.

Davknapp · 2024-07-09T14:06:30Z

src/t8_forest/t8_forest_cxx.cxx

-    for (jt = forest->first_local_tree, count_elements = 0; jt <= forest->last_local_tree; jt++) {
-      tree = (t8_tree_t) t8_sc_array_index_locidx (forest->trees, jt - forest->first_local_tree);
-      tree_class = tree->eclass = t8_cmesh_get_tree_class (forest->cmesh, jt - first_ctree);
+    t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);


Suggested change

t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);

const t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);

Davknapp · 2024-07-09T14:07:16Z

src/t8_forest/t8_forest_cxx.cxx

      eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);
      count_elements++;
-      for (et = start + 1; et < end; et++, count_elements++) {
-        element_succ = t8_element_array_index_locidx_mutable (telements, et - start);
+      for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {


Please change the name of the variable.

lukasdreyer added 2 commits March 25, 2024 08:52

Introduce populate functionality according to cmesh borders

4a6b447

Remove debug print statements with warnings

a576755

CsatiZoltan reviewed Mar 25, 2024

View reviewed changes

lukasdreyer added 2 commits March 25, 2024 16:21

Merge branch 'main' into feature-populate_without_repartition

a7d0677

Correct test offsets

aca7d18

lukasdreyer added 2 commits March 25, 2024 18:56

Fix global to local offset for lagrange geometry

f4a73e4

Add test for quads with lagrange geometry

d96024c

jmark added discussion draft Enhance the visibility that this is a draft. labels Apr 22, 2024

Merge branch 'main' into feature-populate_without_repartition

f93958f

sandro-elsweijer assigned Davknapp Jul 8, 2024

Davknapp reviewed Jul 8, 2024

View reviewed changes

lukasdreyer force-pushed the feature-populate_without_repartition branch from cc579aa to f93958f Compare July 9, 2024 05:42

Davknapp reviewed Jul 9, 2024

View reviewed changes

Davknapp added priority: medium Should be solved within half a year priority: low Should be solved eventually and removed priority: medium Should be solved within half a year labels Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce populate functionality according to cmesh borders #998

Introduce populate functionality according to cmesh borders #998

lukasdreyer commented Mar 25, 2024

CsatiZoltan Mar 25, 2024

lukasdreyer Mar 25, 2024

CsatiZoltan Mar 26, 2024

CsatiZoltan commented Mar 25, 2024 •

edited

Loading

Davknapp left a comment

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

Davknapp Jul 8, 2024

lukasdreyer Jul 9, 2024

Davknapp Jul 9, 2024

Davknapp Jul 9, 2024

Davknapp Jul 9, 2024

Davknapp Jul 9, 2024

Davknapp Jul 9, 2024

Davknapp Jul 9, 2024

	eclass_scheme->t8_element_set_linear_id (element, forest->set_level, start);
	eclass_scheme->t8_element_set_linear_id (element, forest->set_level, 0);

	for (t8_locidx_t et = start + 1; et < end; et++, count_elements++) {
	for (t8_locidx_t et = 1; et < end; et++, count_elements++) {

	t8_element_t *element_succ = t8_element_array_index_locidx_mutable (telements, et - start);
	t8_element_t *element_succ = t8_element_array_index_locidx_mutable (telements, et);

	t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);
	const t8_gloidx_t cmesh_first_tree = t8_cmesh_get_first_treeid (forest->cmesh);

	t8_gloidx_t cmesh_last_tree = cmesh_first_tree + t8_cmesh_get_num_local_trees (forest->cmesh) - 1;
	const t8_gloidx_t cmesh_last_tree = cmesh_first_tree + t8_cmesh_get_num_local_trees (forest->cmesh) - 1;

	t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);
	const t8_gloidx_t first_ctree = t8_cmesh_get_first_treeid (forest->cmesh);

Introduce populate functionality according to cmesh borders #998

Are you sure you want to change the base?

Introduce populate functionality according to cmesh borders #998

Conversation

lukasdreyer commented Mar 25, 2024

General

Tests

Github action

Scripts and Wiki

Licence

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CsatiZoltan commented Mar 25, 2024 • edited Loading

Davknapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CsatiZoltan commented Mar 25, 2024 •

edited

Loading