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! :)
| sc_MPI_Comm comm = sc_MPI_COMM_WORLD; | ||
|
|
||
| /* Print a starting message. */ | ||
| t8_global_productionf (" [tutorial] \n"); |
There was a problem hiding this comment.
We should also think of something different than "tutorial" here. Any ideas?
Co-authored-by: lenaploetzke <70579874+lenaploetzke@users.noreply.github.com>
lenaploetzke
left a comment
There was a problem hiding this comment.
Review adapt callback file
| @@ -21,18 +21,26 @@ | |||
| */ | |||
|
|
|||
| /** \file t8_mesh_element_data.cxx | |||
| * This is the same as general/t8_step5_element_data.cxx but using the mesh handle interface instead of the forest | |||
| * This is step5 of the t8code tutorials. | |||
| * Therefor, this is the same as general/t8_step5_element_data.cxx but using the mesh handle interface instead of the forest | |||
There was a problem hiding this comment.
| * Therefor, this is the same as general/t8_step5_element_data.cxx but using the mesh handle interface instead of the forest | |
| * Therefore, this is the same as general/t8_step5_element_data.cxx but using the mesh handle interface instead of the forest |
| @@ -21,18 +21,26 @@ | |||
| */ | |||
|
|
|||
| /** \file t8_mesh_element_data.cxx | |||
| * This is the same as general/t8_step5_element_data.cxx but using the mesh handle interface instead of the forest | |||
| * This is step5 of the t8code tutorials. | |||
There was a problem hiding this comment.
| * This is step5 of the t8code tutorials. | |
| * This is step5 of the mesh handle tutorials. |
| #include <mesh_handle/mesh.hxx> /** General Mesh header. Always needed for mesh_handle code. */ | ||
| #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> |
There was a problem hiding this comment.
| #include <mesh_handle/concepts.hxx> | |
| #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.
Could you also add this to other tutorials where needed?
| @@ -100,7 +73,7 @@ build_mesh (sc_MPI_Comm comm, int level) | |||
| mesh_handle->set_balance (); | |||
| mesh_handle->set_partition (); | |||
| mesh_handle->set_adapt ( | |||
| TMeshClass::template mesh_adapt_callback_wrapper<user_data> (adapt_callback<TMeshClass>, adapt_data)); | |||
| TMeshClass::template mesh_adapt_callback_wrapper<adapt_data> (&default_adapt_callback<TMeshClass>, adapt_params)); | |||
There was a problem hiding this comment.
Could you please explain shortly why the & is valid here? :) I am not saying that it is wrong, just wondering why i left it out and why it is fine this way.
| sc_MPI_Comm comm = sc_MPI_COMM_WORLD; | ||
|
|
||
| /* Print a starting message. */ | ||
| t8_global_productionf (" [tutorial] \n"); |
There was a problem hiding this comment.
This [tutorial] is not good. Maybe [Step 3 Mesh handle]?
There was a problem hiding this comment.
Please also adapt this in all other tutorials
|
Could you please have a look at the failing workflows? |
| #include <mesh_handle/mesh_io.hxx> /** Used to export mesh to vtk files. */ | ||
| #include <mesh_handle/concepts.hxx> | ||
| #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.
| #include "t8_mesh_tutorials_common.hxx" /** Default adaption function. */ | |
| #include "t8_mesh_tutorials_common.hxx" /** Adaption function definition used for this tutorial. */ |
| #include <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ | ||
| #include "t8_mesh_tutorials_common.hxx" /** Default adaption function. */ | ||
| #include <memory> | ||
| #include <span> |
There was a problem hiding this comment.
| #include <span> |
Not used anymore
| #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 <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ |
There was a problem hiding this comment.
| #include <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ |
unused
| /* Generate a hybrid hypercube, made out of cubes, prisms etc. */ | ||
| auto mesh = t8_mesh_handle::handle_hypercube_hybrid_uniform_default<TMeshClass> (level, comm); | ||
| /* Defining the adaption parameters. */ | ||
| 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 }; |
I think with struct it is C code and without is modern cpp
| mesh->set_balance (); | ||
| mesh->set_partition (); |
There was a problem hiding this comment.
Why is balance, partition and ghost here?
|
|
||
| /* Committing the initial mesh. */ | ||
| mesh->commit (); |
There was a problem hiding this comment.
You dont need to commit before doing something
| /* Creating the adapted mesh as a copy of the initial mesh (Initial mesh can't be refined because it is already committed.) */ | ||
| auto mesh_adapt = std::make_unique<mesh_type> (*mesh); | ||
|
|
There was a problem hiding this comment.
I think it would be good to have seperate functions for this just as in step 3.
| /* Partitioning the mesh.*/ | ||
| mesh_partition_balance->set_partition (); |
There was a problem hiding this comment.
Balance makes no sense after adapting only once. You will need 2 times adaption for this to see a result. ( or recursive but this si not possible for the mesh handle at the moment)
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).