Feature: Add interpolate data functionality to mesh handle - #2382
Feature: Add interpolate data functionality to mesh handle#2382lenaploetzke wants to merge 26 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
==========================================
+ Coverage 82.36% 82.44% +0.08%
==========================================
Files 125 126 +1
Lines 20708 20806 +98
==========================================
+ Hits 17056 17154 +98
Misses 3652 3652 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
spenke91
left a comment
There was a problem hiding this comment.
Well, that was a nice dive into modern C++ template magic! 🪄
Took some time to get it, but I think the code is quite well designed. Great work @lenaploetzke !
I only have a bunch of small remarks and questions. To avoid spamming, I mostly refrained from adding hyphen-related comments; instead I would suggest the following replacements
user defined -> user-defined
index based -> index-based
span based -> span-based
element data handling -> element-data handling
But enough of being picky! Great feature, thanks a lot! 🙂
``
| t8_global_infof ("No interpolation context set.\n"); | ||
| } | ||
| } | ||
| else { | ||
| t8_global_infof ("The element data was not interpolated during adaptation. Use set_element_data() to provide " | ||
| "new data or use the mesh competence interpolate_element_data_mesh_competence.\n"); |
There was a problem hiding this comment.
Should this even be errors?
There was a problem hiding this comment.
Maybe a user prefers to set new element data via set_element_data instead of using the competence? But yes, if we use the competence and do not provide a callback, i will throw an error.
| /** Function that checks if a competence for the interpolation of element data is given. | ||
| * \return true if mesh has the competence, false otherwise. | ||
| */ | ||
| static constexpr bool | ||
| has_interpolate_data_competence () | ||
| { | ||
| return requires (SelfType& mesh) { mesh.set_partition_called (); }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this really a clean way of checking it has the competence? Seems strange and weird to maintain to me 🤔
There was a problem hiding this comment.
Yes i finally found a way better solution!
| static std::unordered_map<t8_forest_t, std::unique_ptr<mesh_interpolate_context_base>>& | ||
| get_map () | ||
| { | ||
| static std::unordered_map<t8_forest_t, std::unique_ptr<mesh_interpolate_context_base>> map; |
There was a problem hiding this comment.
The whole std::unorder_map construct seems a little over-engineered to me. Out of curiosity: Do we already know whether we might ever register more than one forest-context pair at a time? If not, maybe we could make this whole registry more lightweight. On the other hand, I also don't mind having it a little too generic and flexible...
There was a problem hiding this comment.
Maybe we have two meshes at the same time in an example and because of the static, they use the same registry. I think for this, the unordered map is maybe necessary
Co-authored-by: spenke91 <thomas.spenke@dlr.de>
Closes #2383
This also Closes #2115 as we dont need this functionality for the feature (expected before) and also PR #2248 is not needed anymore.
I think we can also
Closes #1654
as this completes the core functionality.
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).