#56: implement parallel meshes as smaller instances of a global mesh #57

pierrepebay · 2022-08-05T15:21:43Z

Resolves #56

- User defined global mesh x and y decomposition still needs to be implemented - Mesh chunk border types aren't set properly

… size Parallel mesh sizes are now more equally distributed

- needs handling of cases where parallel mesh is of multiple current location types at once (e.g. domain mesh is divided in (1, 1), (2, 1), etc. parallel meshes) - needs usage of this type in order to correctly define mesh chunk borders

…nk creation - needs (2, 1) and (1, 2) number of parallel meshes case handling - needs further testing

- testing needed for validation

fixes #59: fix docker build failing

cz4rs · 2022-08-17T17:04:27Z

Commit formatting check will keep complaining until all (non-merge) commits start with #issue_number:

Every commit must match these patterns: 'Merge*' OR '^#[0-9]+[: ]'

cz4rs · 2022-08-17T17:13:59Z

cppfd/CMakeLists.txt

+    target_link_libraries(parallel_test MPI::MPI_CXX)
+    target_link_libraries(allTests MPI::MPI_CXX)
+elseif(${USE_MPI} AND NOT MPI_FOUND)
+    MESSAGE(WARNING "MPI not found. Disabling MPI.")


As long as MPI is marked as REQUIRED, CMake should fail with adequate error in the find_package step.

cz4rs · 2022-08-17T17:49:17Z

cppfd/test.cpp

+  uint64_t n_c_x = 32;
+  uint64_t n_c_y = 32;
+  uint64_t n_parallel_meshes_x = 1;
+  uint64_t n_parallel_meshes_y = 1;
+  uint64_t n_colors_x = 1;
+  uint64_t n_colors_y = 1;


Should these be printed as Input parameters as well? (see below)

cz4rs · 2022-08-17T17:54:11Z

cppfd/solver.cpp

+      for(uint64_t ky = 0; ky < this->n_p_mesh_y; ky++){
+        n_y = n_cells_p_mesh_y;
+        // correct parallel mesh size with remainder
+        if(r_y > 0){
+          n_y++;
+          r_y--;
+        }
+
+        // compute location type of current parallel mesh
+        if(ky == 0){
+          p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_BOT);
+        }
+        else if(ky == this->n_p_mesh_y - 1){
+          p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_TOP);
+        }
+        else{
+          p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_MID);
+        }
+
+        if(this->verbosity >= 2){
+          std::cout << "coordinates, pmesh border type: "
+          << "(0, " << ky << ")"
+          << " // " << static_cast<int>(p_mesh_border) << '\n';
+        }
+
+        // construct current parallel mesh
+        this->parallel_meshes.emplace
+          (std::piecewise_construct,
+          std::forward_as_tuple(std::array<uint64_t,2>{0, ky}),
+          std::forward_as_tuple(n_cells_p_mesh_x, n_y, this->h, n_p, n_q, p_mesh_border, o_x, o_y));
+
+        o_y += n_y * this->h;
+      }


Clarity / method length: extract this into a separate helper method. This will make the whole top-level code much more readable:

if() if() do_only_one_column_of_parallel_meshes(...); elseif() do_only_one_row_of_parallel_meshes(...); else do_something(...); else if() do_parallel_mesh_is_unique(...); ... ... ...

cz4rs · 2022-08-17T18:00:14Z

cppfd/solver.cpp

+      for(uint64_t kx = 0; kx < this->n_p_mesh_x; kx++){
+        n_x = n_cells_p_mesh_x;
+        // correct parallel mesh size with remainder
+        if(r_x > 0){
+          n_x++;
+          r_x--;
+        }
+
+        // compute location type of current parallel mesh
+        if(kx == 0){
+          p_mesh_border = static_cast<int>(LocationIndexEnum::HORIZ_BAR_L);
+        }
+        else if(kx == this->n_p_mesh_x - 1){
+          p_mesh_border = static_cast<int>(LocationIndexEnum::HORIZ_BAR_R);
+        }
+        else{
+          p_mesh_border = static_cast<int>(LocationIndexEnum::HORIZ_BAR_MID);
+        }
+
+        if(this->verbosity >= 2){
+          std::cout << "coordinates, pmesh border type: "
+          << "( " << kx << ", 0)"
+          << " // " << static_cast<int>(p_mesh_border) << '\n';
+        }
+
+        // construct current parallel mesh
+        this->parallel_meshes.emplace
+          (std::piecewise_construct,
+          std::forward_as_tuple(std::array<uint64_t,2>{kx, 0}),
+          std::forward_as_tuple(n_x, n_cells_p_mesh_y, this->h, n_p, n_q, p_mesh_border, o_x, o_y));
+
+        o_x += n_x * this->h;
+      }


Same as above: extract into a helper method, do the same for the remaining cases below.

cz4rs · 2022-08-17T18:11:14Z

cppfd/solver.cpp

+
+        // compute location type of current parallel mesh
+        if(ky == 0){
+          p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_BOT);


Since p_mesh_border is int8_t:

Suggested change

p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_BOT);

p_mesh_border = static_cast<int8_t>(LocationIndexEnum::VERT_BAR_BOT);

here and everywhere below.

cz4rs · 2022-08-17T18:17:56Z

Latest Kokkos release is 3.6.01.
Consider updating it here:

cfd-mini-app/ubuntu-cpp.dockerfile

Line 55 in 03e7615

RUN ./kokkos.sh 3.1.01 /pkgs 0

Note: maybe this could be done in a separate issue?

cz4rs · 2022-08-17T18:26:47Z

cppfd/CMakeLists.txt

@@ -9,6 +9,7 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR})

 # Setting options
 option(OUTPUT_VTK_FILES "When OFF allows for a non-VTK build" OFF)
+option(USE_MPI "When OFF allows for a non-MPI build" OFF)


With this being OFF by default, this feature will not get tested in CI.
Long-term it would be really good to have that configuration tested as well.

cz4rs · 2022-08-17T18:54:19Z

cppfd/mesh_chunk.h

-  EDGE_2 = 6,
-  EDGE_3 = 7,
-  INTERIOR = 8
+  CORNER_0 = 0, // Bottom Left


Consider changing the names instead of adding comments.

cz4rs · 2022-08-17T19:17:03Z

cppfd/parallel_mesh.h

@@ -28,6 +50,7 @@ class ParallelMesh
    uint64_t get_n_points_y() const { return this->n_cells_y + 1; }
    double get_cell_size() const { return this->h; }
    std::array<double,2> get_origin() const {return this->origin; }
+    uint8_t get_location_type() const { return this->location_type; }


Return type does not match int8_t location_type.

Suggested change

uint8_t get_location_type() const { return this->location_type; }

int8_t get_location_type() const { return this->location_type; }

cz4rs · 2022-08-17T19:32:48Z

cppfd/solver.cpp

+  uint64_t n_cells_p_mesh_x = this->domain_size_x / this->n_p_mesh_x;
+  uint64_t n_cells_p_mesh_y = this->domain_size_y / this->n_p_mesh_y;
+
+  uint64_t remainder_x = this->domain_size_x % n_cells_p_mesh_x;
+  uint64_t remainder_y = this->domain_size_y % n_cells_p_mesh_y;


Consider making this constexpr getter methods.

cz4rs · 2022-08-17T19:33:27Z

cppfd/solver.cpp

+  // // Get the number of processes
+  // int p;
+  // MPI_Comm_size(MPI_COMM_WORLD, &p);
+  //
+  // // Get process rank
+  // int world_rank;
+  // MPI_Comm_rank(MPI_COMM_WORLD, &world_rank);


Remove if this is not needed.

cz4rs · 2022-08-17T19:40:14Z

cppfd/solver.cpp

+  if(this->verbosity >= 1){
+    std::cout << "Assembling parallel meshes..." << std::endl;
+  }


Encapsulate this with a helper method like:

void debug_print(const std::string& msg) { if(this->verbosity >= 1){ std::cout << msg << std::endl; } }

so you can use:

debug_print("Assembling parallel meshes...");

cz4rs

Various code quality comments posted. I have only briefly looked at the math / logic here, but I assume that part is OK.

pierrepebay added 5 commits July 25, 2022 15:24

Added MPI library integration

6fd0bea

#54: added basic MPI environment

291f6b9

#55: added basic mpi velocity send receive functions (unfinished)

d0de73b

#56: Progress checkpoint, basic mesh decomposition working

7f5157b

- User defined global mesh x and y decomposition still needs to be implemented - Mesh chunk border types aren't set properly

#56: progress checkpoint: all decomposition cases are handled

54f64c4

pierrepebay added the enhancement New feature or request label Aug 5, 2022

pierrepebay requested a review from lifflander August 5, 2022 15:21

pierrepebay self-assigned this Aug 5, 2022

pierrepebay and others added 13 commits August 8, 2022 12:10

#56: modified p-mesh sizing when domain size isn't multiple of p-mesh…

9948566

… size Parallel mesh sizes are now more equally distributed

#56: progress checkpoint: added border type case handling in mesh chu…

5cf7505

…nk creation - needs (2, 1) and (1, 2) number of parallel meshes case handling - needs further testing

#56: completed domain decomposition (all cases handled)

61f391f

- testing needed for validation

#59: changed ubuntu version

e4af7ea

#59: modified clang version

d6bc27e

#59: fixed solver code that was producing errors in docker build

b480eeb

#59: fixed cmake linking

34c3efa

#59: fixed other cmake linking

2755bcd

#59: fixed includes

9f14167

#59: fixed preprocessor directives

9337874

#59: added missing preprocessor directives

641adca

Merge pull request #60 from DARMA-tasking/59-docker-build-failing

e3b505d

fixes #59: fix docker build failing

pierrepebay marked this pull request as ready for review August 15, 2022 13:24

pierrepebay requested a review from cz4rs August 16, 2022 17:18

nmm0 mentioned this pull request Aug 16, 2022

Meeting Agenda [do not close] DARMA-tasking/vt#925

Open

cz4rs reviewed Aug 17, 2022

View reviewed changes

cppfd/mesh_chunk.h

EDGE_2 = 6,

EDGE_3 = 7,

INTERIOR = 8

CORNER_0 = 0, // Bottom Left

Copy link

cz4rs Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing the names instead of adding comments.

cz4rs reviewed Aug 17, 2022

View reviewed changes

cz4rs requested changes Aug 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#56: implement parallel meshes as smaller instances of a global mesh #57

#56: implement parallel meshes as smaller instances of a global mesh #57

pierrepebay commented Aug 5, 2022

cz4rs commented Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022 •

edited

Loading

cz4rs Aug 17, 2022 •

edited

Loading

cz4rs Aug 17, 2022

cz4rs commented Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs Aug 17, 2022

cz4rs left a comment

	p_mesh_border = static_cast<int>(LocationIndexEnum::VERT_BAR_BOT);
	p_mesh_border = static_cast<int8_t>(LocationIndexEnum::VERT_BAR_BOT);

	uint8_t get_location_type() const { return this->location_type; }
	int8_t get_location_type() const { return this->location_type; }

#56: implement parallel meshes as smaller instances of a global mesh #57

Are you sure you want to change the base?

#56: implement parallel meshes as smaller instances of a global mesh #57

Conversation

pierrepebay commented Aug 5, 2022

cz4rs commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cz4rs Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

cz4rs Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cz4rs commented Aug 17, 2022

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

cz4rs left a comment

Choose a reason for hiding this comment

cz4rs Aug 17, 2022 •

edited

Loading

cz4rs Aug 17, 2022 •

edited

Loading