Component `extract_subset` #426

ahighmoon · 2023-10-05T21:50:41Z

Create a component: to extract a subset/submesh of a given mesh according to a given tag

daniel-zint · 2023-10-13T19:44:51Z

this PR should be a draft

daniel-zint

I think the general idea of the algorithm itself is there but there are some issues on the coding side. Every method will get a Mesh as input and will return a Mesh as output. You are currently still using the IGL style matrices. They should be avoided if possible.

Oh and please set this PR as "draft".

daniel-zint · 2023-10-20T15:31:01Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

+    int nb_vertex_in = 0;
+    int nb_tri_in = tag.size();
+    std::vector<bool> vertices_in_bool(nb_vertex);
+    for (int k = 0; k < nb_vertex; ++k) {vertices_in_bool[k] = false;}


It looks like you are not using an auto formatter. We have a .clang-format file in our repository. Please install an auto formatter and use this format file. I recommend also to activate "format on save". That way it is sure that you never forget formatting before you commit.

daniel-zint · 2023-10-20T15:35:46Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

@@ -62,23 +62,34 @@ wmtk::TriMesh extract_subset_2d(


 // Note: the above is a draft version of the algo, implemented in bad, drafted data structure
-// The following is new code to be finished, commented out to compile
+// The following is new code to be finished
 wmtk::TriMesh extract_subset_2d(std::vector<wmtk::Tuple> vertices, std::vector<wmtk::Tuple> triangles, std::vector<size_t> tag){


I don't really understand the parameters here. Why vertices and triangles? I think you should have a Mesh as input in combination with an Attribute that is used as tag.

Minor comment:
The return value should also just be a Mesh. This code can be written in a generic enough way so that it works for extracting triangles or just a bunch of edges. However, we can do the generalization as soon as the basic concept is there, so don't worry about that for now.

daniel-zint · 2023-10-20T15:37:25Z

components/wmtk_components/extract_subset/extract_subset.cpp

+
+namespace wmtk{
+namespace components {
+wmtk::TriMesh extract_subset(long dimension, const wmtk::TriMesh& m, std::vector<size_t> tag){


Change the order of parameters: mesh, tag, dimension

daniel-zint · 2023-10-20T15:38:52Z

components/wmtk_components/extract_subset/extract_subset.cpp

+            // return internal::extrace_subset_2d();
+            std::vector<Tuple> vertices = m.get_all(wmtk::PrimitiveType::Vertex);
+            std::vector<Tuple> triangles = m.get_all(wmtk::PrimitiveType::Face);
+            return internal::extract_subset_2d(vertices, triangles, tag);


A Tuple without it's corresponding mesh is useless. The tuple is a handle that can only be evaluated in combination with its mesh.

daniel-zint · 2023-10-20T15:39:41Z

components/wmtk_components/extract_subset/extract_subset.cpp

+            std::vector<Tuple> triangles = m.get_all(wmtk::PrimitiveType::Face);
+            return internal::extract_subset_2d(vertices, triangles, tag);
+        }
+        case 3: {


Add a default case that contains throw "not implemented"; or something similar.

Btw if you throw a string literal it causes weird outputs. Make sure to throw a runtime error

daniel-zint · 2023-10-20T15:44:06Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

+
+    //tag the preserved ones and count number of vertex in new extraction
+    for (size_t k = 0; k < nb_tri_in; ++k){
+        for (size_t k2 = 0; k2 < 3; ++k2) {


I don't know why you are using k and k2 here but the convention is to use i and j. Please change that, otherwise it might be confusing.

daniel-zint · 2023-10-20T15:46:17Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

+
+    // construct a map from old vertex id to new new id
+    std::map<int, int> old2new;
+    int j = 0;


j is more than a counter variable here, give it a more descriptive name.

daniel-zint · 2023-10-20T15:47:03Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

+    wmtk::TriMesh mesh;
+    wmtk::RowVectors3l tris;
+    tris.resize(nb_tri_in, 3);
+    for (unsigned int i = 0; i < nb_tri_in; ++i){


should also be size_t

daniel-zint · 2023-10-20T15:48:07Z

components/wmtk_components/extract_subset/internal/extract_subset_2d.cpp

+    points_in.resize(nb_vertex_in, 2);
+    for (int i = 0; i < nb_vertex; ++i){
+        if (vertices_in_bool[i]){
+            points_in(old2new[i], 0) = points[i][0];


points_in.row(i) = points[i]

daniel-zint · 2023-10-20T15:50:24Z

tests/components/test_component_extract_subset.cpp

+#include <catch2/catch_test_macros.hpp>
+
+
+TEST_CASE("manual test case", "[components][extract_subset][2D]")


Test cases should not have spaces in their name, use _ instead: manual_test_case.

Furthermore, test case names should be very descriptive because all test cases share the same scope.

mtao · 2023-10-28T19:56:56Z

What is the progress here? Given the new multimesh structure there's extra things to develop in this concept of extracting a subset. There are others working on the extraction of a subset with codimension so it'd be good to unify the API for both/make sure both implementations have the appropriate similarities

…file

daniel-zint

Please take a look at the code I referenced for the usage of attributes.

daniel-zint · 2023-11-06T21:35:30Z

tests/components/test_component_extract_subset.cpp


-
-TEST_CASE("manual test case", "[components][extract_subset][2D]")
+TEST_CASE("3 neighbors test case", "[components][extract_subset][2D]")


write test case names with underscores: "3_neighbors_test_case"

daniel-zint · 2023-11-06T21:38:34Z

tests/components/test_component_extract_subset.cpp


+    // NOTE: I don't know why the following line gives me a linking error of undefined reference...
+    // wmtk::TriMesh new_tm = wmtk::components::extract_subset(tm, tag_handle, 2);
+    wmtk::TriMesh new_tm = wmtk::components::internal::extract_subset_2d(tm, tag_handle);


If I see it correctly, you didn't set extract_subset_2d in the internal namespace. That is why it cannot find it.

daniel-zint · 2023-11-06T21:43:06Z

tests/components/test_component_extract_subset.cpp

+    V.row(1) << 0; // ont tri is not tagged
+    V.row(2) << 1;
+    V.row(3) << 1;
+    auto tag_handle = wmtk::mesh_utils::set_matrix_attribute(V, "tag", wmtk::PrimitiveType::Face, tm);


That way of setting values should be only used if you want to assign a value to each vertex at once. It is mostly for the case that you load data from a file. In general, you should follow this example:

wildmeshing-toolkit/tests/test_accessor.cpp

Line 54 in 764ca0e

TEST_CASE("test_accessor_basic")

register_attribute can also take a default value for its attribute:

wildmeshing-toolkit/src/wmtk/Mesh.hpp

Line 97 in 764ca0e

MeshAttributeHandle<T> register_attribute(

You might want to use that to default everything to something signaling invalidness like -1.

According to the commit history f1b7140, this functionality is update on Oct 5. So in our current branch there is no such interface right now.

I did a merge just now.

mtao · 2023-11-09T23:03:32Z

this task seems quite similar to #502
@daniel-zint do you want to take a look and determine how these two PRs should interact?

…equire a vector

codecov · 2023-11-10T19:32:19Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (754f335) 85.46% compared to head (de1533e) 85.54%.
Report is 1345 commits behind head on main.

❗ Current head de1533e differs from pull request most recent head 5a7757b. Consider uploading reports for the commit 5a7757b to get more accurate results

Files	Patch %	Lines
.../wmtk_components/extract_subset/extract_subset.cpp	72.00%	7 Missing ⚠️
...ents/extract_subset/internal/extract_subset_2d.cpp	98.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
+ Coverage   85.46%   85.54%   +0.07%     
==========================================
  Files         210      212       +2     
  Lines        5987     6086      +99     
==========================================
+ Hits         5117     5206      +89     
- Misses        870      880      +10

Flag	Coverage Δ
wildmeshing	`85.54% <91.01%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ented and everything needs tests

…ifications

…ical endpoints? or split on midpoint

…ions

…my submesh files

…ofaces

…hing now in one internal file

…troublesome to do if-statement everywhere in the second half of the algo

…ons:)

… to the new algorithm, finish but with bug

…_manifold_edge test case runs with "duplicate index conflict" Runtime Error: tet#2 corner local id 2 with gid 2 has already got assigned index = 2 before adjusting

… attribute by local vid, instead of order of gettting them

…ed print statements

…to the # of face since there is one more region outside

…han 8 tets. NEED visualization component

ahighmoon added 2 commits September 28, 2023 05:05

create dummy component extract subset&CMakeLists

3aa11fb

finish 2d extract subset by convert old vectices id to new

3921e20

daniel-zint linked an issue Oct 13, 2023 that may be closed by this pull request

Component extract_subset #403

Open

daniel-zint assigned ahighmoon and daniel-zint Oct 13, 2023

ahighmoon added 2 commits October 18, 2023 23:37

correct extract_subset_2d interface w/o implementation

1be4189

failed attempt to add fv/vf accessors

dcf2723

daniel-zint requested changes Oct 20, 2023

View reviewed changes

ahighmoon marked this pull request as draft November 1, 2023 17:20

change argument to MeshAttributeHandle, w/ one linking error in test …

5504abb

…file

daniel-zint reviewed Nov 6, 2023

View reviewed changes

ahighmoon added 2 commits November 9, 2023 10:50

Merge branch 'main' into dzint/component_extract_subset

476bb22

finish a version of ds adaption w/ linking error from external

472a219

ahighmoon added 2 commits November 10, 2023 00:12

fix linking error and remove all unnecessary comments

06e9114

change test case to 2d tet and change argument of extract_subset to r…

3203b4c

…equire a vector

ahighmoon added 11 commits November 12, 2023 01:01

fix internal bug and pass all tag tests on 2d tet

b6c5f36

add comments and another geometric config for test

de1533e

init the topology algo, compiles and links fine

db6d98c

finish 1st version of topo algo, compiles, bdry function to be implem…

d299091

…ented and everything needs tests

remove unnecessary bdry determine func, add manual test case of 3+4

886909a

fixing calcing cc using dfs recursion

b4b4d7f

finish finding vertex copies and ready for reconstruct mesh

da5b811

finish mesh reconstruction

9faab15

change helper functions to inline in the header, some performance mod…

407b2b5

…ifications

move utils functions back to cpp file

183fabc

init the test validness & manifold functions

1fc1002

ahighmoon added 30 commits December 6, 2023 07:47

save temp work in step 2 of topo_separate_3d

777cd32

copy & adapt the 2d case to 3d. question: to create dup edge on ident…

bcfd83e

…ical endpoints? or split on midpoint

finish topo_3d algo, extremely messy but should be correct

cb53ea9

init the 3d_manifold test function

fdd4b5b

refactor the function signature to handle 2d and 3d all at once

73cfe5e

[NOT proven correct] add first version of is_disk and is_sphere funct…

c08131c

…ions

remove redundant dimension argument, fix wrong use of m.capacity()

961cc25

move prev to attic & implement algo in new_topo_separate & create dum…

ad34515

…my submesh files

save work, attempt to combine 2 functions into new_topo_sep

1d757f1

add initial check for only processing tagged top simplices

d3f23a4

finish new_topo_sep middle part, did experiments with top_dimension_c…

5d89c64

…ofaces

finish reconstruction part

931b10c

remove files unneed

3afccbe

restore utils.cpp temporarily, seg fault

2c56235

restore the gen_submesh file since they are actually needed

29327cd

[Save work] algo ok, but compile/linking error

ed177b2

finish the extract_submesh task without considering positions, everyt…

be76d8b

…hing now in one internal file

Add coordinates preserving section

f8ecc61

Move submesh functionality back to its own file; Note: because it is …

be2b55a

…troublesome to do if-statement everywhere in the second half of the algo

finish main algorithm, test on 3+4 test case and passes all 4 asserti…

6cb1ed2

…ons:)

add coordinates preserving logic to topo_sep function

92445ac

remove unused accessor argument of topo_sep

e656a30

minor changes

4a8c362

[Save Temp work] in the middle of adjusting generate_submesh function…

106aca6

… to the new algorithm, finish but with bug

finish generate_submesh, test passes on 3+4 2D case without coordinates

bdcdfc1

[Save Temp Work] restored some test cases in the test file, now 2_non…

0b8e485

…_manifold_edge test case runs with "duplicate index conflict" Runtime Error: tet#2 corner local id 2 with gid 2 has already got assigned index = 2 before adjusting

fix bug! in topo_sep func, should ALWAYS access and change the vertex…

6cfce0b

… attribute by local vid, instead of order of gettting them

restore the test case of 6_cycle_tets in 3D, passed it; remove unneed…

3b5353c

…ed print statements

change the way to determine planar graph : in calc euler char, add 1 …

63e9562

…to the # of face since there is one more region outside

add 3D random test case, now reports error for every mesh with more t…

5a7757b

…han 8 tets. NEED visualization component

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Component `extract_subset` #426

Component `extract_subset` #426

ahighmoon commented Oct 5, 2023

daniel-zint commented Oct 13, 2023

daniel-zint left a comment •

edited

Loading

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

mtao Oct 28, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

daniel-zint Oct 20, 2023

mtao commented Oct 28, 2023

daniel-zint left a comment

daniel-zint Nov 6, 2023

daniel-zint Nov 6, 2023

daniel-zint Nov 6, 2023

ahighmoon Nov 9, 2023

ahighmoon Nov 9, 2023

mtao commented Nov 9, 2023

codecov bot commented Nov 10, 2023 •

edited

Loading

		#include <catch2/catch_test_macros.hpp>


		TEST_CASE("manual test case", "[components][extract_subset][2D]")



		TEST_CASE("manual test case", "[components][extract_subset][2D]")
		TEST_CASE("3 neighbors test case", "[components][extract_subset][2D]")

Component extract_subset #426

Are you sure you want to change the base?

Component extract_subset #426

Conversation

ahighmoon commented Oct 5, 2023

daniel-zint commented Oct 13, 2023

daniel-zint left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtao commented Oct 28, 2023

daniel-zint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtao commented Nov 9, 2023

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

Component `extract_subset` #426

Component `extract_subset` #426

daniel-zint left a comment •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading