Testing Framework for Hull #875

Kushal-Shah-03 · 2024-07-22T20:19:01Z

@elalish and @pca006132, I've cleaned the code for the testing framework, keeping the tests of a High Quality Sphere, Menger Sponge and the script to run the code on the Thingi10K dataset, and then get the statistics for the implementations. I've attached instructions as to how to run the code in the comments. I would appreciate it if you could review the code once as well.

As for instructions:
To run the executable (run this in ./build directory)
Usage: ./test_hull_performance <Test (Sphere/Menger/Input)> <Implementation (Hull/Hull_CGAL)> <Print Header (1/0)> <Input Mesh (filename)>
Example: ./extras/testHullPerformace Menger Hull 1
The Input test allows the user to enter a custom file to run the implementation on. You can test our current implementation as well as CGAL implementation. The Print Header refers to printing the corresponding header for the csv file (it's mainly used by the script for the dataset, for single testing purposes just use 1 there)

To run the script for the dataset (run this is ./extras directory)
Usage : ./run.sh ../build/extras/testHullPerformance {path_to_dataset_folder} {name_of_csv} {implementation(Hull,Hull_CGAL)}
example : ./run.sh ../build/extras/testHullPerformance ./Thingi10K/raw_meshes/ Hull.csv Hull

Kushal-Shah-03 · 2024-07-22T20:23:03Z

I've tried my best to include only the relevant things from our experiments, if there's something I missed or something I should remove. I would gladly make the change.

pca006132 · 2024-07-23T15:44:11Z

extras/test_hull_performace.cpp

+  return;
+}
+
+void PrintManifold(Manifold input, Manifold (Manifold::*hull_func)() const) {


Why should this take a manifold method? I think you can just make it take a generic function that returns a manifold given a set of points.

pca006132 · 2024-07-23T15:44:30Z

extras/test_hull_performace.cpp

+  std::cout << elapsed.count() << " sec";
+}
+
+void PrintCGAL(Manifold input, Manifold (Manifold::*hull_func)() const) {


We probably don't need a separate CGAL one as well.

The reason, I kept it, is since we had discussed that we don't want to keep the other implementations in our code. CGAL, seemed like the simplest way to compare the outputs relatively.

I mean, we can just use the function for PrintManifold? Just run CGAL hull in the hull_func function

Right, since we don't need the Manfiold Method function, we can just use that. I will change that too. Now, that I think of it, that was a very redundant piece of code thanks for pointing it out.

Ok so, since we aren't storing the output of CGAL as a manifold, I am just computing the CGAL output, and then printing the outputs using the CGAL functions. I didn't want to convert the CGAL output back to manifold as that would add time overhead, so what should I do in this case?

The simplest way would be to do the time measurement in the callback, or you can split the function into two, the actual hull operation as well as a postprocessing operation which returns a manifold, and you only measure the time in the actual hull operation.

class HullImpl { public: // actual hull operation, we will measure the time needed to evaluate this function. virtual void hull(const std::vector<glm::vec3>& points); // return the result of the hull operation, should be called after `hull`. virtual manifold::Manifold result(); };

And just let the implementations inherit this class. (or if you use template, you don't need inheritance).

extras/run.sh

pca006132 · 2024-07-23T15:48:18Z

I think we also talked about checking if the convex hull result is a valid convex polyhedron? I think that is a pretty important thing to test for.

elalish

We don't need to bother with Draft PRs - our CI will only run on regular ones.

elalish · 2024-07-23T15:30:41Z

bindings/python/manifold3d.cpp

@@ -107,7 +107,7 @@ struct nb::detail::type_caster<glm::mat<C, R, T, Q>> {
  static handle from_cpp(glm_type mat, rv_policy policy,
                         cleanup_list *cleanup) noexcept {
    T *buffer = new T[R * C];
-    nb::capsule mem_mgr(buffer, [](void *p) noexcept { delete[] (T *)p; });
+    nb::capsule mem_mgr(buffer, [](void *p) noexcept { delete[](T *) p; });


This is due to using the wrong version of clang-format. Make sure you've pulled master.

I will try it again, thanks for pointing it out.

Could you guide me on how to fix this, I had used the latest commit from master on the main repository.

What clang-format version are you using? Use a newer one, e.g. 16.

elalish · 2024-07-23T15:50:54Z

extras/test_hull_performace.cpp

+    if (!strcmp(argv[2], "Hull"))
+      PrintManifold(inputManifold, &Manifold::Hull);
+    else if (!strcmp(argv[2], "Hull_CGAL"))
+      PrintCGAL(inputManifold, &Manifold::Hull);


It looks like you're always using Manifold::Hull() - do we still need this input?

Oh right, I forgot that we didn't need input of the hull function there, as we're using CGAL, I'll make that change, thanks for pointing it out. And the reason I am taking this input, is that we can run a comparison between various implementations (or for the bare minimum at least with CGAL).

elalish · 2024-07-23T15:52:29Z

extras/test_hull_performace.cpp

+// Constructs a high quality sphere, and tests the convex hull implementation on
+// it (you can pass the specific hull implementation to be tested). Comparing
+// the volume and surface area with CGAL implementation
+void SphereTestHull(Manifold (Manifold::*hull_func)() const,


We talked about how this one can test our precision by checking if the number of verts is the same between input and output.

I am also printing the number of vertices of the original Manifold and the Hull in the output, I just forgot to mention that in the comment, I'll re-frame the comment. Also, do we want to add this test into hull_test.cpp as well?

elalish · 2024-07-23T15:53:44Z

CMakeLists.txt

@@ -46,6 +46,7 @@ endif()
 option(TRACY_ENABLE "Use tracy profiling" OFF) 
 option(TRACY_MEMORY_USAGE "Track memory allocation with tracy (expensive)" OFF)
 option(BUILD_TEST_CGAL "Build CGAL performance comparisons" OFF)
+option(TEST_HULL_PERFORMANCE "Test Hulling Algorithm" OFF)


I think I'd rather build this under the TEST_CGAL flag instead of adding another.

Alright, I will change that.

elalish · 2024-07-23T15:55:07Z

extras/test_hull_performace.cpp

+// (you can pass the specific hull implementation to be tested). Comparing the
+// volume and surface area with CGAL implementation, for various values of
+// rotation
+void MengerTestHull(Manifold (Manifold::*hull_func)() const, float rx, float ry,


This is fine for comparison to CGAL, but I'm actually more interested in adding to our own testing framework. Can you build tests like this directly into hull_test.cpp for just Manifold's implementation?

So we just want to keep this testing script for the tests on datasets, and these single tests, I can shift to hull_test.cpp?

Kushal-Shah-03 · 2024-07-25T10:56:58Z

I think we also talked about checking if the convex hull result is a valid convex polyhedron? I think that is a pretty important thing to test for.

I had created the check for this in the implementation of the algorithm itself, so that I could perform a check for each iteration while testing. While moving the implementation to our code, I am working on adding that function as well.

Kushal-Shah-03 · 2024-07-31T14:52:57Z

I've made the changes as suggested, except adding the check for the valid convex hull, since I was debating whether we wanted that in the code with the implementation that we're moving to our library or in this testing framework, what's your opinion on it?

codecov · 2024-07-31T15:16:59Z

Codecov Report

Attention: Patch coverage is 42.42424% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (d437097) to head (3dcbae0).
Report is 69 commits behind head on master.

Files	Patch %	Lines
test/hull_test.cpp	42.42%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #875      +/-   ##
==========================================
- Coverage   91.84%   89.90%   -1.94%     
==========================================
  Files          37       64      +27     
  Lines        4976     9216    +4240     
  Branches        0      960     +960     
==========================================
+ Hits         4570     8286    +3716     
- Misses        406      930     +524

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

pca006132 · 2024-08-03T12:51:23Z

Just ping me know when I should review it.

Kushal-Shah-03 · 2024-08-03T16:55:06Z

Just ping me know when I should review it.

I'm currently working on the reduced points cases that Emmett had suggested, but I'm having difficulties with it, I've added the check for valid convex hull. If you plan to start the refactoring of the code soon, I can commit this much for now. And we can add these failing cases later?

pca006132 · 2024-08-04T07:24:35Z

No hurry, I just asked because I saw the conflicts are now resolved and tests seem to be passing. You can do it later.

elalish · 2024-08-05T16:35:24Z

I think I'd like to land this PR before #881, since it's good to have regression tests in place for a big change like that. What troubles are you having with the other test cases? I would expect some to fail for now (you can mark them as e.g. TEST(Hull, DISABLED_HardTest). You can reenable them with your fixes in #881.

Kushal-Shah-03 · 2024-08-05T17:03:22Z

I think I'd like to land this PR before #881, since it's good to have regression tests in place for a big change like that. What troubles are you having with the other test cases? I would expect some to fail for now (you can mark them as e.g. TEST(Hull, DISABLED_HardTest). You can reenable them with your fixes in #881.

I was having issues with the recreating the error, with the points values (not storing them in binary). I am working on that now, I hope to resolve it soon and I will add the tests as you suggested. I will focus on this PR then, considering what we had planned for #881 is pretty much finished

… points

Kushal-Shah-03 · 2024-08-06T20:25:49Z

I've added the test for the valid convex hull in test_hull_performance.cpp, and the two cases for the reduced points in the file hull_test.cpp. What do we want to test for the currently failing tests, whether they are giving correct volume and area, or whether the output is a valid convex hull? Depending on that I will add the check for convex hull in hull_test.cpp if needed.

Also, for checking if for any input file the output hull is valid or not, I'm using the MANIFOLD_DEBUG flag for now, should I change that in any way, and do you want that function to compile only when that flag is enabled?

elalish

This is looking good - let's just do these small updates and get it merged.

elalish · 2024-08-06T21:31:24Z

test/hull_test.cpp

@@ -94,3 +94,54 @@ TEST(Hull, Sphere) {
  EXPECT_FLOAT_EQ(sphereHull.GetProperties().volume,
                  sphere.GetProperties().volume);
 }
+
+// TEST(Hull, FailingTest1) {


Instead of commenting these tests, you can change them to TEST(Hull, DISABLED_FailingTest1) - this way they get compiled, but just not run, and we get a warning to remind us.

elalish · 2024-08-06T21:40:37Z

test/hull_test.cpp

+//       {-20.442623138f, -35.407661438f, 8.2749996185f},
+//       {10.229375839f, -14.717799187f, 10.508025169f}};
+//   auto hull = Manifold::Hull(hullPts);


Yes, for these odd shapes I think testing IsConvex() is the way to go.

Kushal-Shah-03 · 2024-08-07T06:57:11Z

I've made the suggested changes (DISABLED in tests and check for convex Mesh in hull_test.cpp), I also added the definition of isMeshConvex in hull_test_performance.cpp under the MANFIOLD_DEBUG flag.

elalish

Thanks!

Kushal-Shah-03 added 2 commits July 23, 2024 01:38

Testing Framework for Hull

dabea81

Fixed Formatting

94dfcaf

elalish marked this pull request as ready for review July 23, 2024 15:27

pca006132 reviewed Jul 23, 2024

View reviewed changes

elalish requested changes Jul 23, 2024

View reviewed changes

Kushal-Shah-03 added 2 commits July 31, 2024 20:19

Made Changes Based on the Review

12a7afa

Fixed Formatting

fd1869a

Fixed Formatting Using clang-16

a29e071

Kushal-Shah-03 added 5 commits August 1, 2024 00:59

Reduced NumSegs for Sphere to fit within memory

3b7885f

Reducing NumSegs Further

78d6824

Reducing SphereSegs Further

f42513d

Fixed Formatting

8fbe6fd

Check for Valid Convex Mesh

adfbc57

Added Check for Valid Convex Hull, and two failing cases with reduced…

3249f89

… points

elalish requested changes Aug 6, 2024

View reviewed changes

Added DISABLED in tests and isMeshConvex check in hull_tests.cpp

3dcbae0

elalish approved these changes Aug 7, 2024

View reviewed changes

elalish merged commit 75c340f into elalish:master Aug 7, 2024
21 checks passed

Kushal-Shah-03 mentioned this pull request Aug 7, 2024

Removing QuickHull dependency #881

Merged

elalish mentioned this pull request Nov 5, 2024

v3.0 #1024

Merged

Testing Framework for Hull #875

Testing Framework for Hull #875

Conversation

Kushal-Shah-03 commented Jul 22, 2024

Kushal-Shah-03 commented Jul 22, 2024

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

pca006132 commented Jul 23, 2024

elalish 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

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

Kushal-Shah-03 commented Jul 25, 2024

Kushal-Shah-03 commented Jul 31, 2024

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

pca006132 commented Aug 3, 2024

Kushal-Shah-03 commented Aug 3, 2024

pca006132 commented Aug 4, 2024

elalish commented Aug 5, 2024 • edited Loading

Kushal-Shah-03 commented Aug 5, 2024

Kushal-Shah-03 commented Aug 6, 2024

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kushal-Shah-03 commented Aug 7, 2024

elalish left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2024 •

edited

Loading

elalish commented Aug 5, 2024 •

edited

Loading