-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MultiMesh Refactor #409
MultiMesh Refactor #409
Conversation
multimesh things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good. There is just quite a lot of commented out code that maybe can be removed.
Otherwise, my main issue is the inconsistency in the usage of Simplex
and Tuple
. I think that should be unified.
// Simplex maps | ||
//=========== | ||
// generic mapping function that maps a tuple from "this" mesh to the other mesh | ||
std::vector<Simplex> map(const Mesh& my_mesh, const Mesh& other_mesh, const Simplex& my_simplex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should start using Doxygen comments.
|
||
static std::vector<Simplex> find_all_simplices_in_child_mesh(const Mesh& parent_mesh, long child_id, const Simplex& simplex_parent); | ||
Simplex map_to_parent(const Mesh& my_mesh, const Simplex& my_simplex) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You return Simplex
in some functions and Tuple
in others. I am OK with both but I feel like that should be somehow consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i try to make sure that the tuple variants are called _tuple
but this shuld be documented better
}; | ||
|
||
private: | ||
Mesh* m_parent = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected some sort of smart pointer here. I can't tell though if that makes the code any better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be a std::weak_ptr i guess - but doing this would require making every Mesh
a shared_ptr (and also disabling public construction of meshes)
src/wmtk/MultiMeshManager.hpp
Outdated
// bool is_child_mesh_valid(const Mesh& my_mesh, const Mesh& child_mesh) const; | ||
|
||
//// checks that the map is consistent | ||
// bool is_child_map_valid(const Mesh& my_mesh, const ChildData& child) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these functions still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, i will delete - they got moved to unit test (DEBUG_MultiMeshManager)
{ | ||
// this is just to do a little redirection for simpplifying map_to_child (and potentially for a | ||
// visitor pattern) | ||
return map_to_child_tuples(my_mesh, m_children.at(child_id), my_simplex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe assert existence of child_id
in m_children
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a std::vector and using at
in debug mode makes an exception if it's not there
src/wmtk/MultiMeshManager.cpp
Outdated
MultiMeshManager::MultiMeshManager(MultiMeshManager&& o) = default; | ||
MultiMeshManager& MultiMeshManager::operator=(const MultiMeshManager& o) = default; | ||
MultiMeshManager& MultiMeshManager::operator=(MultiMeshManager&& o) = default; | ||
auto [source_mesh_base_tuple, target_mesh_base_tuple] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
@@ -389,11 +390,14 @@ TriMesh::TriMeshOperationExecutor::prepare_operating_tuples_for_child_meshes() c | |||
} | |||
} | |||
} | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete old code?
|
||
namespace { | ||
|
||
std::vector<PrimitiveType> find_local_switch_sequence_trimesh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if multimesh requires a utils subfolder. To me, everything in the multimesh folder seems like utility functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH i want to move MultiMeshManager into this folder - and tehre's some more utilities i'm currently working on that might belong here
} | ||
|
||
TEST_CASE("test_split_multi_mesh","[multimesh][2D]") | ||
/* | ||
TEST_CASE("test_split_multi_mesh", "[multimesh][2D]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this test case come back or is it dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working on for next pr
|
||
|
||
namespace wmtk::tests { | ||
void DEBUG_MultiMeshManager::check_child_mesh_valid(const Mesh& my_mesh, const Mesh& child_mesh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should perform some checks in the future, right? In that case I would prefer the "throw a message" thing and a CHECK(false)
to make sure that other people are not using it without being aware that it does not do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont know - i can't remember who/what the original purpose was for this and just copied it over. tired :(
Codecov Report
@@ Coverage Diff @@
## main #409 +/- ##
=======================================
Coverage ? 81.88%
=======================================
Files ? 154
Lines ? 4156
Branches ? 0
=======================================
Hits ? 3403
Misses ? 753
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. |
The current multimesh interface is optimized for tri-tri mappings and not ready for the other mappings we will soon assert. It also has too many public interfaces.
This PR aims to allow for only 2 basic functions:
a) Add a new mesh (for components / apps to use)
b) map a simplex from one mesh to another (which is a one-to-many relationship)
There will also be 2 convenience functions for when users know direct relationships (in debug mode these relationships will be asserted upon):
c) map to parent(parent_mesh, Tuple)
d) map to child(child_mesh, Tuple)
NOTE: I am breaking / giving up on the TriMeshOperations working with multimesh in this PR. Too much further work si required and I want to get this PR in