Feature: Very first version of the unstructured mesh interface #1829
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1829 +/- ##
==========================================
+ Coverage 76.52% 76.60% +0.08%
==========================================
Files 106 109 +3
Lines 18655 18728 +73
==========================================
+ Hits 14275 14347 +72
- Misses 4380 4381 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sandro-elsweijer
left a comment
sandro-elsweijer
left a comment
There was a problem hiding this comment.
First part of my review :)
sandro-elsweijer
left a comment
sandro-elsweijer
left a comment
There was a problem hiding this comment.
Looks very nice!
Mainly, the comments are about documentation and variable naming, so nothing about the general concept.
But one thing we should think about is the derefencing and the probably memory leaks.
Co-authored-by: Sandro Elsweijer <[email protected]>
Feature: Interface as cmake library
…nto rename-unstructured-mesh-interface
Improvement: Rename unstructured mesh interface
|
For me, the APIs of t8code are the C, Fortran, Python... APIs. So interfaces to different programming languages. |
Co-authored-by: Sandro Elsweijer <[email protected]>
It does not feel right for me to put the handle in the src folder but i am fine with putting it in its own. The idea was that the mesh handle is an api for other software so i think it is not really wrong in the folder. What do you think @spenke91 ? |
| */ | ||
|
|
||
| /** \file element.hxx | ||
| * Definition of the elements used in the mesh class. |
There was a problem hiding this comment.
| * Definition of the elements used in the mesh class. | |
| * Definition of the elements used in the \ref t8_mesh_handle::mesh class. |
|
|
||
| namespace t8_mesh_handle | ||
| { | ||
| /* Forward declaration of the mesh class of the handle. |
There was a problem hiding this comment.
| /* Forward declaration of the mesh class of the handle. | |
| /* Forward declaration of the \ref t8_mesh_handle::mesh class of the handle. |
| std::vector<t8_3D_point> vertex_coordinates; | ||
| vertex_coordinates.reserve (num_corners); | ||
| for (int icorner = 0; icorner < num_corners; ++icorner) { | ||
| t8_3D_point vertex; | ||
| t8_forest_element_coordinate (m_mesh->m_forest, m_tree_id, element, icorner, vertex.data ()); | ||
| vertex_coordinates.push_back (vertex); | ||
| } |
There was a problem hiding this comment.
saves a copy/move
In the future, we can also ask for all points of an element at the same time, this should also be faster. But for now this is alright :)
| std::vector<t8_3D_point> vertex_coordinates; | |
| vertex_coordinates.reserve (num_corners); | |
| for (int icorner = 0; icorner < num_corners; ++icorner) { | |
| t8_3D_point vertex; | |
| t8_forest_element_coordinate (m_mesh->m_forest, m_tree_id, element, icorner, vertex.data ()); | |
| vertex_coordinates.push_back (vertex); | |
| } | |
| std::vector<t8_3D_point> vertex_coordinates (num_corners); | |
| for (int icorner = 0; icorner < num_corners; ++icorner) { | |
| t8_forest_element_coordinate (m_mesh->m_forest, m_tree_id, element, icorner, vertex_coordinates[icorner].data ()); | |
| vertex_coordinates.push_back (vertex); | |
| } |
| * This function uses or sets the cached version defined in TCompetence if available and calculates if not. | ||
| * \return Vector with one coordinate array for each vertex of the element. | ||
| */ | ||
| std::vector<t8_3D_point> |
There was a problem hiding this comment.
Shouldn't we make this const? Otherwise the user can alter the coordinates in the cache.
| //--- Getter for the member variables. --- | ||
| /** | ||
| * Getter for the tree id of the mesh element. | ||
| * \return The element's tree id. |
There was a problem hiding this comment.
| * \return The element's tree id. | |
| * \return The element's local tree id. |
| /** | ||
| * Getter for the eclass of the mesh element. | ||
| * \return The element's eclass. | ||
| */ | ||
| t8_eclass_t | ||
| get_tree_class () const | ||
| { | ||
| return t8_forest_get_tree_class (m_mesh->m_forest, m_tree_id); | ||
| } |
There was a problem hiding this comment.
we should not confuse the eclass and element shape
| /** | |
| * Getter for the eclass of the mesh element. | |
| * \return The element's eclass. | |
| */ | |
| t8_eclass_t | |
| get_tree_class () const | |
| { | |
| return t8_forest_get_tree_class (m_mesh->m_forest, m_tree_id); | |
| } | |
| /** | |
| * Getter for the eclass of the tree of the mesh element. | |
| * \return The eclass of the element's tree. | |
| */ | |
| t8_eclass_t | |
| get_tree_class () const | |
| { | |
| return t8_forest_get_tree_class (m_mesh->m_forest, m_tree_id); | |
| } | |
| /** | |
| * Getter for the element's shape. | |
| * \return The shape of the element. | |
| */ | |
| t8_element_shape | |
| get_shape () const | |
| { | |
| return t8_forest_get_scheme (m_mesh->m_forest)->element_get_shape (get_tree_class(), get_element()); | |
| } |
| /** | ||
| * Returns an iterator to the first (local) mesh element. | ||
| * \return Iterator to the first (local) mesh element. | ||
| */ | ||
| mesh_iterator | ||
| begin () | ||
| { | ||
| return m_elements.begin (); | ||
| } | ||
|
|
||
| /** | ||
| * Returns an iterator to a mesh element following the last (local) element of the mesh. | ||
| * \return Iterator to the mesh element following the last (local) element of the mesh. | ||
| */ | ||
| mesh_iterator | ||
| end () | ||
| { | ||
| return m_elements.end (); | ||
| } |
There was a problem hiding this comment.
Should we have a non-const iterator in the first place? All data is read-only and all caches are marked mutable.
| /** | ||
| * Getter for a mesh element given its local index. | ||
| * \param [in] local_index The local index of the element to access. | ||
| * \return Reference to the mesh element. | ||
| */ | ||
| TMeshElement& | ||
| operator[] (t8_locidx_t local_index) | ||
| { | ||
| return m_elements[local_index]; | ||
| } |
There was a problem hiding this comment.
Same here, I do not think the user should get a non-const reference
| if (!m_elements.empty ()) { | ||
| m_elements.clear (); | ||
| } | ||
| m_elements.reserve (get_num_local_elements ()); |
There was a problem hiding this comment.
this is already optimal. clear does not do anything if the vector is empty.
| if (!m_elements.empty ()) { | |
| m_elements.clear (); | |
| } | |
| m_elements.reserve (get_num_local_elements ()); | |
| m_elements.clear (); | |
| m_elements.reserve (get_num_local_elements ()); |
| const t8_scheme* scheme; | ||
| t8_eclass_t eclass; |
There was a problem hiding this comment.
These are not used in the test and do not need to be member variables
Describe your changes here:
Closes #1655
This PR implements the very basic structure of the unstructured mesh interface. It provides only the level variable. Other variables should be implemented in future PRs.
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
script/find_all_source_files.scpto check the indentation of these files.License
doc/(or already has one).