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>
| 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| /** \file t8_mesh_element_data.cxx |
lenaploetzke
left a comment
There was a problem hiding this comment.
Review adapt callback file
| @@ -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.
There was a problem hiding this comment.
In this case it's optional, because set_adapt() uses an std::function as it's "adapt_callback_type", when you use &default_... you transfer the function pointer to create the std::function, if you leave it out (default_...) c++ automatically converts this to a function pointer implicitly to create the std::function.
So essentially, it's just implicit vs explicit handover of the function pointer.
I just added it here when i implemented the adapt_callback as its own file, because i used it in the other 2 tutorials like this aswell, so it's uniform.
There was a problem hiding this comment.
Nice, thank you for the explanation. In general, explicit coding is better in most cases, so yes, & is even better! :)
|
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.
| #include <mesh_handle/mesh_io.hxx> | ||
| #include <mesh_handle/concepts.hxx> | ||
| #include <t8_types/t8_vec.hxx> | ||
| #include <mesh_handle/mesh.hxx> /** General Mesh header. Always needed for mesh_handle code. */ |
There was a problem hiding this comment.
| #include <mesh_handle/mesh.hxx> /** General Mesh header. Always needed for mesh_handle code. */ | |
| #include <mesh_handle/mesh.hxx> /** General mesh header. Always needed for mesh_handle code. */ |
| #include <mesh_handle/concepts.hxx> | ||
| #include <t8_types/t8_vec.hxx> | ||
| #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. */ |
There was a problem hiding this comment.
| #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/competence_pack.hxx> /** Competence pack for basic mesh_handle features. Look into tutorials/mesh_handle/t8_mesh_competences for more information. */ |
| #include <t8_types/t8_vec.hxx> | ||
| #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. */ |
There was a problem hiding this comment.
| #include <mesh_handle/constructor_wrappers.hxx> /** Wrapper for basic Cmesh to mesh_handle conversions. */ | |
| #include <mesh_handle/constructor_wrappers.hxx> /** Wrapper for basic cmesh to mesh_handle conversions. */ |
| 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| /** \file default_adapt_callback.hxx |
There was a problem hiding this comment.
| /** \file default_adapt_callback.hxx | |
| /** \file t8_mesh_tutorials_common.hxx |
| * In this example we want to adapt in a spherical shape around a given point. */ | ||
| struct adapt_data | ||
| { | ||
| t8_3D_vec midpoint; /**< midpoint of our sphere. */ |
There was a problem hiding this comment.
| t8_3D_vec midpoint; /**< midpoint of our sphere. */ | |
| t8_3D_vec midpoint; /**< Midpoint of our sphere. */ |
| t8_global_productionf (" [t8 step 4 Mesh handle] \n"); | ||
| t8_global_productionf (" [t8 step 4 Mesh handle] Finished all steps successfully.\n"); | ||
| t8_global_productionf (" [t8 step 4 Mesh handle] \n"); | ||
|
|
There was a problem hiding this comment.
This tutorial throws errors...
|
|
||
| /** Helper function to create a partitioned and balanced mesh from an initial mesh. | ||
| * \param mesh The initial mesh to adapt. | ||
| * \param adapt_params The adaptation parameters to use for the adaptation. |
There was a problem hiding this comment.
| * \param adapt_params The adaptation parameters to use for the adaptation. |
| /* Balancing the mesh. */ | ||
| mesh_partition_balance->set_balance (); | ||
|
|
||
| /* Committing the partitioned and balanced mesh. */ |
There was a problem hiding this comment.
Not really. set_balance does not really balance the mesh. This is done at commiting. I think you can adapt it if you understand my comment at step 3
|
|
||
| /** Helper function to create a ghost mesh from an initial mesh. | ||
| * \param mesh The initial mesh to adapt. | ||
| * \return A unique pointer to the new mesh with ghost layers. |
There was a problem hiding this comment.
| * \return A unique pointer to the new mesh with ghost layers. | |
| * \return A unique pointer to the new mesh with a ghost layer. |
| return mesh_partition_balance; | ||
| } | ||
|
|
||
| /** Helper function to create a ghost mesh from an initial mesh. |
There was a problem hiding this comment.
| /** Helper function to create a ghost mesh from an initial mesh. | |
| /** Helper function to create a mesh with ghosts from an initial mesh. |
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).