-
Notifications
You must be signed in to change notification settings - Fork 17
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
SimplicialComplex test autogen #361
Conversation
class Mesh: | ||
def __init__(self): | ||
self.vertices = [] # List of vertices | ||
self.tetrahedra = [] # List of tetrahedra |
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.
why switch for tets? i assume the class is supposed to be agnostic to tri vs tet meshes?
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.
tets will alway be the top simplex, so there will be no redundant tets in init.
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.
sure, but why not just use sets and tuples in this implementation instead of switching back and forth between things?
# Write test code | ||
with open(output_file_name, 'w') as file: | ||
write_cpp_headers(file) | ||
for mesh_name in mesh_names: |
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 write one file per mesh, i think these will get rather large and it's better to split the compilation up. If we want to optimize the sizes of our outputs I think we should change how the test files are written (like having a table + unit tests that compare against a table rather than repeat the word REQUIRE and CHECK so many times).
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.
that would be a future issue/PR
|
||
|
||
# Parameters | ||
path = '../../data/' |
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 path might not be canonical so it should be an argument for the script with some instructions on what should be found in the path (such as being some subfolder of WMTK_DATA_DIR?)
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 need to double check if all cases are covered by the tests. Otherwise, I added some minor comments.
RowVectors3d V; | ||
RowVectors3l F; | ||
std::string name = "/circle.obj"; | ||
std::string path; |
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.
use std::filesystem::path
std::vector<std::vector<long>> DEBUG_TriMesh::get_sorted_simplicial_complex(const std::vector<Simplex> &sc) const | ||
{ | ||
std::vector<std::vector<long>> ret; | ||
for (auto s : sc) |
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.
for(const auto& s : sc)
@@ -174,4 +173,41 @@ auto DEBUG_TriMesh::get_tmoe(const Tuple& t, Accessor<long>& hash_accessor) | |||
{ | |||
return TriMeshOperationExecutor(*this, t, hash_accessor); | |||
} | |||
|
|||
std::vector<std::vector<long>> DEBUG_TriMesh::get_sorted_simplicial_complex(const std::vector<Simplex> &sc) 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.
The name of this function is not very intuitive. Please, either provide a better name or add a comment describing what this function does.
Tuple t; | ||
std::vector<std::vector<long>> sc_v, sc_e, sc_f; | ||
t = m.tuple_from_id(PV, 5058); | ||
sc_v = m.get_sorted_simplicial_complex(SimplicialComplex::open_star(m, Simplex(PV, t)).get_simplex_vector()); |
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'd prefer const auto sc_v = ...
here.
{ | ||
RowVectors3d V; | ||
RowVectors3l F; | ||
std::string name = "/circle_0.7.obj"; |
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 don't think that file is in the data repo
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.
There is a code to generate this, but I am not sure if we want this in the data repo
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 belongs in the data repo - separate your code for auto-generating meshes and for validating meshes into two parts:
- generation goes in teh data repo + some generated meshes
- test generation lives in
tests/auto_generated/
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.
There are some function tests missing.
path = '../../data/' | ||
mesh_names = ['circle.obj', 'bunny.obj','blub.obj'] | ||
output_file_name = 'test_simplicial_complex_2d.cpp' | ||
test_function_names = ['open_star', 'closed_star', 'link', 'simplex_with_boundary', 'top_coface_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.
missing functions that should be tested
boundary
simplices_w_boundary_intersect
link_cond
vertex_one_ring
k_ring
for _ in range(n_bd_samples): | ||
index = random.choice(indices) # Choose a random index | ||
v_samples.append((bd_loop[index],)) | ||
e_samples.append(tuple(sorted([bd_loop[index], bd_loop[(index + 1) % len(bd_loop)]]))) |
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 should also test edges that are incident to the boundary, i.e. one vertex on the boundary and one on the interior. The same holds for faces.
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.
Also, there might be edges that are interior but both vertices are on the boundary. Any possible combination should be tested.
also, auto_generated should just be our scripts folder - which already handles our other autogen code |
add autogenerated tests for SimplicalComplex tests for 2d