Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2348 +/- ##
=======================================
Coverage 82.80% 82.80%
=======================================
Files 139 139
Lines 21584 21584
=======================================
Hits 17872 17872
Misses 3712 3712 ☔ 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>
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>
lenaploetzke
left a comment
There was a problem hiding this comment.
Thanks for your work :) I am sorry for all the comments but i think consistency is really important for tutorials that a beginner should learn with :)
| Create a coarse mesh, output it to vtu and destroy it. We need a coarse mesh to initialize our mesh handle mesh. | ||
|
|
||
| [step2] - | ||
| [step2] (mesh_handle/t8_mesh_step2_uniform_mesh.cxx) - |
There was a problem hiding this comment.
I think the space between ] ( is too much. In the readme, this is not clickable :)
| [step4] (mesh_handle/t8_mesh_step4_partition_balance_ghost.cxx) - | ||
| Partitioning, balancing and creating a ghost layer for a mesh. | ||
|
|
||
| [step5](mesh_handle/t8_mesh_step5_element_data.cxx) - |
There was a problem hiding this comment.
Moreover i thinkk the mesh_handle/ is too much. You can check this in github by just switching the branch. Please check that all links work.
|
|
||
| /* Printing the mesh information. */ | ||
| print_stats_and_export (*mesh, "Ghost mesh", prefix_ghost); | ||
| int ghost_elements = mesh->get_num_ghosts (); |
There was a problem hiding this comment.
This also does not return type int right
| /** This is not doing anything here, because we only adapt once before this line, | ||
| * so the difference between elements is +1 or -1 at most. | ||
| * We still include it here for demonstration purposes. | ||
| */ |
There was a problem hiding this comment.
| /** This is not doing anything here, because we only adapt once before this line, | |
| * so the difference between elements is +1 or -1 at most. | |
| * We still include it here for demonstration purposes. | |
| */ |
Sorry i think this is not true although i said it... Because you can of course coarsen one element and refine its neighbor so we have a level diff of 2
| t8_global_productionf (" [mesh_step4] Total elements: %li \n", global_elements); | ||
|
|
||
| /* Writing the mesh to vtu and pvtu files, using the extended version of the function to ensure additional data like ghost elements, treeid etc. to be written into the files. */ | ||
| t8_mesh_handle::write_mesh_to_vtk_ext (mesh, prefix, 0, nullptr, true, true, true, true, true, false, false); |
There was a problem hiding this comment.
There are many bools, can you maybe add inline comments like /* write_treeid */ true, /* write_mpirank */ true
?
| t8_global_productionf (" [mesh_step4] Adapt the mesh.\n"); | ||
| t8_global_productionf (" [mesh_step4] \n"); | ||
|
|
||
| /** Call adaption helper function. */ |
There was a problem hiding this comment.
This /** style comments are only for declarations for doxygen. So please use normal /* comments inside function bodies.
| #include <mesh_handle/mesh.hxx> /** General mesh header. Always needed for mesh_handle code. */ | ||
| #include <mesh_handle/mesh_io.hxx> /** Used to export mesh to vtk files. */ | ||
| #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 constrain the template parameters to only allow mesh handle classes. */ |
There was a problem hiding this comment.
concept is not used here because of your using mesh type
| #include <mesh_handle/mesh_io.hxx> /** Used to export mesh to vtk files. */ | ||
| #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 constrain the template parameters to only allow mesh handle classes. */ | ||
| #include <t8_types/t8_vec.hxx> /** t8 vector dataclass. */ |
There was a problem hiding this comment.
Do you use this here explicitly?
| * \param [in] mesh The initial mesh to adapt. | ||
| * \param [in] adapt_params The adaptation parameters to use for the adaptation. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Why the blank lines? Please remove these everywhere in the file
| * outside of a given sphere. | ||
| * | ||
| * \tparam TMeshClass The mesh handle class. | ||
| * \param[in] mesh The mesh that should be adapted. |
There was a problem hiding this comment.
I think everywhere else in the tutorials you used \param [in] right so with a space
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).