Improvement: Element data for mesh handle is now defined via mesh competence#2242
Improvement: Element data for mesh handle is now defined via mesh competence#2242lenaploetzke wants to merge 4 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## new-handle-partition-and-balance #2242 +/- ##
====================================================================
- Coverage 78.51% 78.49% -0.02%
====================================================================
Files 115 116 +1
Lines 19196 19207 +11
====================================================================
+ Hits 15071 15076 +5
- Misses 4125 4131 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Davknapp
left a comment
There was a problem hiding this comment.
A first round of comments.
| const auto num_local_elements = this->underlying ().get_num_local_elements (); | ||
| const auto num_ghosts = this->underlying ().get_num_ghosts (); | ||
| T8_ASSERT (element_data.size () == static_cast<size_t> (num_local_elements)); | ||
| m_element_data.reserve (num_local_elements + num_ghosts); |
There was a problem hiding this comment.
I think we don't need to reserve the memory for the vector. std::move should only swap pointers here, so we don't need to allocate new memory. However you should mention in the comment, that element_data is moved, not copied.
There was a problem hiding this comment.
I thought i needed the reserve to avoid a reallocation if we later exchange ghost data (therefore i reserve more space then needed for element_data). What do you think?
| this->underlying ().m_mesh->m_element_data.reserve (this->underlying ().m_mesh->get_num_local_elements () | ||
| + this->underlying ().m_mesh->get_num_ghosts ()); | ||
| this->underlying ().m_mesh->m_element_data.resize (this->underlying ().m_mesh->get_num_local_elements ()); | ||
| this->underlying ().m_mesh->m_element_data[this->underlying ().get_element_handle_id ()] = std::move (element_data); |
There was a problem hiding this comment.
The move command should be sufficient, if I get it right that this is a std::vector again.
Otherwise I don't realy understand why you first reserve for num_local_elements + num_ghosts and then resize it to num_local_elements. Shouldn't it be enough to just allocate for num_local_elements (if we need an allocation).
If moving is enough, please tell so in the comment.
There was a problem hiding this comment.
Oh yes i dont need the resize (but still reserve for the ghost elements and move, see above)
Describe your changes here:
Closes #2185. Update of #2184 because something got messed up in git.
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).