Improvement: Mesh handle tutorials - #2348
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2348 +/- ##
=======================================
Coverage 82.28% 82.28%
=======================================
Files 125 125
Lines 20701 20701
=======================================
Hits 17033 17033
Misses 3668 3668 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lenaploetzke
left a comment
There was a problem hiding this comment.
Partial review. Thank you for your work, this is really nice and my comments are mainly documentation! :)
Co-authored-by: lenaploetzke <70579874+lenaploetzke@users.noreply.github.com>
| 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| /** \file t8_mesh_element_data.cxx |
There was a problem hiding this comment.
Could you please adapt this?
lenaploetzke
left a comment
There was a problem hiding this comment.
Review adapt callback file
|
Could you please have a look at the failing workflows? |
lenaploetzke
left a comment
There was a problem hiding this comment.
Most things are just typos and capitalization.
Co-authored-by: lenaploetzke <70579874+lenaploetzke@users.noreply.github.com>
| int rank = 0; | ||
| MPI_Comm_rank (comm, &rank); | ||
| if (rank == 0) { | ||
| std::cout << "=== " << stage << " ===" << std::endl; | ||
| std::cout << "Total elements: " << global_elements << std::endl; | ||
| } |
There was a problem hiding this comment.
Yeah i know, but some pverhead here. get_num_global_elements is merged now so you are ready to use it here :)
| #include <mesh_handle/competence_pack.hxx> /** Competence Pack for basic mesh_handle features. Look into tutorials/mesh_handle/t8_mesh_competences for more information. */ | ||
| #include <mesh_handle/constructor_wrappers.hxx> /** Wrapper for basic Cmesh to mesh_handle conversions. */ | ||
| #include <mesh_handle/mesh_io.hxx> /** Used to export mesh to vtk files. */ | ||
| #include <mesh_handle/concepts.hxx> /** Include this to use c++ concepts related to the mesh handle. This can be used to constraint the template parameters to only allow mesh handle classes. */ |
There was a problem hiding this comment.
This line is too long and i think you can remove some line breaks in the file descriptions . If you want to i can show you how to add a helper for this in vscode :)
|
|
||
| /** Build our adapted mesh by transferring the adaption parameters and adapting once with our \ref adapt_callback function. | ||
| * \tparam TMeshClass The mesh handle class. | ||
| * \param sc_MPI_Comm The MPI Communicator. |
There was a problem hiding this comment.
| * \param sc_MPI_Comm The MPI Communicator. | |
| * \param comm The MPI Communicator. |
| t8_global_productionf (" [t8 step 3 Mesh handle] Creating an adapted mesh.\n"); | ||
| t8_global_productionf (" [t8 step 3 Mesh handle] \n"); | ||
| /* The initial uniform refinement level. */ | ||
| int uniform_level = 3; |
There was a problem hiding this comment.
| int uniform_level = 3; | |
| const int uniform_level = 3; |
Best way is to mark everything const if possible :)
| auto mesh = build_mesh<mesh_type> (comm, uniform_level); | ||
| /* Write the mesh to a vtu file. */ | ||
| t8_global_productionf (" [t8 step 3 Mesh handle] \n"); | ||
| t8_global_productionf (" [t8 step 3 Mesh handle] Writing adapted mesh to vtu file: step3_adapted_mesh.vtu\n"); |
There was a problem hiding this comment.
Would be better to introduce a variable for the name and reusing it! That way if you change the name, you do not have to change it everywhere :)
| int uniform_level = 3; | ||
| /* Building the mesh. */ | ||
| { /** Scope to ensure mesh is deleted properly. */ | ||
| auto mesh = build_mesh<mesh_type> (comm, uniform_level); |
There was a problem hiding this comment.
I noticed that in the general tutorial, the uniform mesh is also stored as a vtk such that you can compare them. Why did you decided against this here?
| * we will now learn how to control the mesh creation in more detail, | ||
| * how to partition and balance a mesh and how to generate a layer of ghost elements. |
There was a problem hiding this comment.
| * we will now learn how to control the mesh creation in more detail, | |
| * how to partition and balance a mesh and how to generate a layer of ghost elements. | |
| * we will now learn how to partition and balance a mesh and how to generate a layer of ghost elements. |
| #include <mesh_handle/constructor_wrappers.hxx> /** Wrapper for basic Cmesh to mesh_handle conversions. */ | ||
| #include <mesh_handle/concepts.hxx> /** Include this to use c++ concepts related to the mesh handle. This can be used to constraint the template parameters to only allow mesh handle classes. */ | ||
| #include <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ | ||
| #include "t8_mesh_tutorials_common.hxx" /** Default adaption function. */ |
There was a problem hiding this comment.
Also not "default" here. Just use the description that you provided in step 3 pls
| #include <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ | ||
| #include "t8_mesh_tutorials_common.hxx" /** Default adaption function. */ | ||
| #include <memory> | ||
| #include <iostream> |
There was a problem hiding this comment.
| #include <iostream> |
| print_mesh_stats (const std::unique_ptr<mesh_type>& mesh, const char* stage, sc_MPI_Comm comm) | ||
| { | ||
| int local_elements = mesh->get_num_local_elements (); | ||
| int global_elements = 0; |
There was a problem hiding this comment.
I think i already wrote is somewhere but please use your new mesh handle function here (already merged)
| t8_global_productionf (" [mesh_step4] Total elements: %i \n", global_elements); | ||
| } | ||
|
|
||
| /** Helper function to create an adapted mesh from an initial mesh. |
There was a problem hiding this comment.
Now you just adapt a mesh and dont create a new adapted mesh. Please change this
| /* Adapting the mesh once with our adapt_callback_sphere function from step 3 and the adapt_params. Both can be found in the file \ref t8_mesh_tutorials_common.hxx. */ | ||
| mesh->set_adapt ( | ||
| mesh_type::template mesh_adapt_callback_wrapper<adapt_data> (&adapt_callback_sphere<mesh_type>, adapt_params)); | ||
| /* Committing the adapted mesh. */ |
There was a problem hiding this comment.
Not really. The mesh gets adapted at the commit step, so its not "the adapted mesh"
| mesh->commit (); | ||
| } | ||
|
|
||
| /** Helper function to create a partitioned and balanced mesh from an initial mesh. |
| void | ||
| create_partitioned_balanced_mesh (const std::unique_ptr<mesh_type>& mesh) | ||
| { | ||
| /* Calculate partition information.*/ |
There was a problem hiding this comment.
Maybe it is good if you have a look at the documentation of these functions and of commit. You do not calculate something, just setting the flags such that the mesh is partitioned at commit call. We can also talk about the commit procedure if you want to, just ask.
| t8_global_productionf (" [mesh_step4] \n"); | ||
|
|
||
| /* The initial uniform refinement level. */ | ||
| int uniform_level = 3; |
There was a problem hiding this comment.
| int uniform_level = 3; | |
| const int uniform_level = 3; |
| int uniform_level = 3; | ||
|
|
||
| /* Parameters for the adaption step. */ | ||
| struct adapt_data adapt_params = { { 0.5, 0.5, 1.0 }, 0.2, 0.4 }; |
There was a problem hiding this comment.
| struct adapt_data adapt_params = { { 0.5, 0.5, 1.0 }, 0.2, 0.4 }; | |
| adapt_data adapt_params = { { 0.5, 0.5, 1.0 }, 0.2, 0.4 }; |
The struct is old c style
| true, false, false); | ||
|
|
||
| /** | ||
| * GHOST MESH |
There was a problem hiding this comment.
At comments you have to pay attention to the indention please,this is not checked by the ci and not corrected by the indent script
| create_ghost_mesh (mesh); | ||
|
|
||
| /* Printing the mesh information. */ | ||
| print_mesh_stats (mesh, "Ghost mesh", comm); |
There was a problem hiding this comment.
Maybe it would be nice to print also the number of ghost elements here (maybe only for one process or so).
| { | ||
| auto mesh_handle = t8_mesh_handle::handle_hypercube_hybrid_uniform_default<TMeshClass> (level, comm); | ||
| struct user_data adapt_data = { | ||
| struct adapt_data adapt_params = { |
There was a problem hiding this comment.
| struct adapt_data adapt_params = { | |
| adapt_data adapt_params = { |
I also just copied the old C style whoopsi
Describe your changes here:
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.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
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
scripts/internal/find_all_source_files.shto check the indentation of these files.License
doc/(or already has one).