Improves robustness of `SignedDistance` query #706

kennyweiss · 2021-11-08T07:45:02Z

Summary

This PR is a bugfix
It resolves Regression in signed distance query #703
It improves the robustness of primal::closest_point(pt, triangle) by adding an EPS tolerance parameter and using fuzzy comparisons to better determine if/when the closest point is on an edge or vertex of the triangle
It uses this in SignedDistance to better track whether the closest point is on an edge or vertex.
For these two cases, we need to track the weighted sum of all triangles incident in the edge or vertex
It also improves how SignedDistance tracks which case it is in (face, edge or vertex)
This improvement was tested against a configuration of the plane_simp.stl triangle mesh that was reported in Regression in signed distance query #703
as well as a new tetrahedron.stl mesh that was added for this PR
Also resolves Add complete set of arithmetic operators for points and vectors #708
Also resolve Add testing of Release Builds in our CI #346

TODO

Check this code against GPU configurations. Some functions will likely need to be host/device decorated
Add regression tests for the new failures (plane_simp and tetrahedron)
Ensure that all quest regression tests are passing
Update RELEASE-NOTES
Update data submodule after merging Adds datasets and regression for debugging SignedDistance query axom_data#8
Enable extra quest regression tests in a subset of CI jobs

rhornung67 · 2021-11-08T17:16:14Z

src/axom/primal/operators/closest_point.hpp

+  using detail::isGeq;
+  using detail::isLeq;
+
+  const PointType& A = tri[0];


👍 much better!

publixsubfan · 2021-11-08T17:50:00Z

src/axom/quest/SignedDistance.hpp

-    // would also require generating cell-to-face connectivity for the surface
-    // mesh.
+    break;
+  default:  // Use precomputed normal for edges and vertices


I remember when debugging the original SignedDistance PR that the BVH traversal sometimes only accumulated one face of an edge pseudo-normal; was that the underlying issue here?

In any case, I think it might be good to have a debug check/tracker variable here for the edge and vertex pseudo-normals:

If we keep track of the number of normals we accumulated for the current weighted sum, an edge closest point should have exactly two accumulated normals, one for each face.

Not sure about the angle-weighted normal for vertex closest points, but I think we could check that there at least 3? (since any angle at a triangle vertex should be less than 180 degrees)

I remember when debugging the original SignedDistance PR that the BVH traversal sometimes only accumulated one face of an edge pseudo-normal; was that the underlying issue here?

There were a few problems. Most prominently:

primal::closest_point() was using strict inequality tests, so points that were within `EPS of an edge or vertex could be marked as interior (face), and either not counted for the edge/vertex test, or would clear out the normal

The logic for determining whether to clear normals and/or sum normals needed some improvement.

The underlying issue is that we're using purely geometric tests, so we cannot guarantee that the we're catching all (both) faces in the star of an edge or all faces in the star of a vertex.

A potentially better, but more expensive way to handle this would be to:

Weld the mesh to ensure that we have a mesh instead of a collection of disjoint faces (see Port vertex welding code to GPUs via RAJA #705)

Compute the pseudo-normal at each vertex of the welded mesh

Only keep track of the single closest point

Derive the sign from the normal of the mesh at this point.

This would robustly handle the face, edge and vertex cases for a watertight surface mesh.
The other downside of this approach would be that it would involve more memory (to store the normals) and more computation time (to weld the mesh and compute the normals)

We can certainly add a debug check/variable to count the number of accumulated normals.
What would you suggest we do with that information though?
I suppose we can add an option to add scalar fields to the output vtk mesh to visualize this (closest point type and count)

bmhan12 · 2021-11-10T17:17:10Z

.gitlab/build_ruby.yml

+    COMPILER: "[email protected]"
+    HOST_CONFIG: "ruby-toss_3_x86_64_ib-${COMPILER}.cmake"
+    BUILD_TYPE: "Release"
+    EXTRA_CMAKE_OPTIONS: "-DAXOM_QUEST_ENABLE_EXTRA_REGRESSION_TESTS:BOOL=ON"


Should options like this and AXOM_ENABLE_MFEM_SIDRE_DATACOLLECTION be documented in our ReadTheDocs configuration options?

Thanks @bmhan12. In general, yes, we should document all our user-facing config options.

In the past, we have intentionally omitted a few niche options from our documentation, including the two that you mentioned:

AXOM_QUEST_ENABLE_EXTRA_REGRESSION_TESTS is a developer option. We have a few quest regression tests that are always on and this option adds additional tests.

AXOM_ENABLE_MFEM_SIDRE_DATACOLLECTION was intended as a short-lived option -- it was added because we couldn't yet enable this feature by default, and we plan to remove this option (and enable the feature by default) as soon as we can. As far as I can tell, we should be able to do this once we have a working hip configuration.

There are a few other undocumented options in the code, like AXOM_MINT_USE_SIDRE that we might also want to consider

axom/src/axom/mint/CMakeLists.txt

Lines 18 to 19 in 3999cf2

cmake_dependent_option( AXOM_MINT_USE_SIDRE "Enables Sidre support in Mint" ON

"AXOM_ENABLE_SIDRE" OFF)

Any of these should be easy enough to add to the docs. We might want to discuss this further as a team. I'll create an issue for it.

Update: #711

kennyweiss · 2021-11-10T20:00:16Z

I've resolved the issues in #703, added more quest regression tests and enabled these in some CI jobs, so this PR should be ready for review.

For completeness, here are the two examples and their isosurfaces from #703

 >./examples/quest_signed_distance_interface_ex -f ../data/quest/plane_simp.stl --box-dims  120 120 60 --box-min  -540 -140 -190 --box-max   540  940  350 --batched -e1

./examples/quest_signed_distance_interface_ex  -f ../data/quest/tetrahedron.stl  --box-dims 100 100 100  --box-min -5 -5 -5 --box-max 5 5 5 --batched -e 1

The plane appears to be underresolved to capture all the features, but the spurious signs (topological noise) have been resolved in both cases, and the signs agree with those of the InOutOctree.

agcapps

@kennyweiss , thank you for the work.

scripts/azure-pipelines/linux-build_and_test.sh

… edges and vertices of the mesh There are noticeable improvements in quest_regression_test against `tetrahedron` and `plane_simple` meshes, but it does not fix all cases. For example, ``` > ./tests/quest_regression_test -m ../data/quest_tetrahedron -r 99 99 99 --min -5 -5 -5 --max 5 5 5 ``` is fixed (went from 202 bad points to 0 bad points), while ``` > ./tests/quest_regression_test -m ../data/quest_tetrahedron -r 100 100 100 --min -5 -5 -5 --max 5 5 5 ``` is improved, but still has errors -- it went from 2125 bad points to 1024 bad points.

…al::Vector

…parisons Using new `EPS` tolerance parameter.

* Use fuzzy comparisons in closest_point calculations to better determine when the closest point is on an edge or a vertex * Improves tests that determine if we should reset the cached sum of normals and to see if we should add a new normal to the current point

…didate struct Since we've improved the robustness of the edge and vertex cases and we're tracking which case we're in, we no longer need a separate vector to track the sum of edge normals and the sum of vertex normals.

Includes some additional minor fixes.

This counter tracks the number of entities at the current closest point. This is expected to be 2 for edges and higher for vertices. Added per PR suggestion.

…uery

…tion

Adds tests for the `tetrahedron` mesh as well as the `boxedSphere` mesh. Also adds a regression configuration for the `plane_simp` mesh, as described in #703

…e job

…default: Debug) Also use `Release` build for `linux_clang10` Azure CI job.

…ld_type` Options are: 'Debug' (default), 'RelWithDebInfo', 'Release' and 'RelMinSize'

…est regression tests

agcapps

Kenny, thank you for your diligent work.

publixsubfan

LGTM - great to see robustness improvements in Primal as well 👍

publixsubfan · 2021-11-15T20:44:13Z

src/axom/quest/examples/quest_signed_distance_interface.cpp

@@ -78,7 +78,7 @@ struct Arguments
  bool use_shared {false};
  bool use_batched_query {false};
  bool ignore_signs {false};
-  quest::SignedDistExec exec_space;
+  quest::SignedDistExec exec_space {quest::SignedDistExec::CPU};


Good catch - I was running into some issues with wonky execution space values on optimized builds as well.

kennyweiss added bug Something isn't working App Integration Issues related to integration with applications Quest Issues related to Axom's 'quest' component labels Nov 8, 2021

kennyweiss added this to the FY22 Development milestone Nov 8, 2021

kennyweiss self-assigned this Nov 8, 2021

rhornung67 reviewed Nov 8, 2021

View reviewed changes

publixsubfan reviewed Nov 8, 2021

View reviewed changes

kennyweiss force-pushed the bugfix/kweiss/signed-distance branch 2 times, most recently from 8409610 to 90252bf Compare November 9, 2021 22:52

kennyweiss marked this pull request as ready for review November 9, 2021 23:01

kennyweiss force-pushed the bugfix/kweiss/signed-distance branch 2 times, most recently from 199b75f to 6520ec9 Compare November 10, 2021 00:31

kennyweiss requested review from joshessman-llnl, white238, agcapps, publixsubfan, bmhan12, cyrush and rhornung67 November 10, 2021 00:42

kennyweiss force-pushed the bugfix/kweiss/signed-distance branch 3 times, most recently from 6bde074 to 5ef1b3d Compare November 10, 2021 02:08

bmhan12 reviewed Nov 10, 2021

View reviewed changes

kennyweiss mentioned this pull request Nov 10, 2021

Review Axom's config options and associated documentation #711

Closed

agcapps approved these changes Nov 11, 2021

View reviewed changes

scripts/azure-pipelines/linux-build_and_test.sh Show resolved Hide resolved

kennyweiss force-pushed the bugfix/kweiss/signed-distance branch from 60d399a to cb2457b Compare November 12, 2021 05:24

kennyweiss added 3 commits November 15, 2021 11:18

Adds standalone operator+ functions to add a primal::Point and prim…

68207bd

…al::Vector

Cleans up implementation of primal::closest_point()

811858c

kennyweiss added 19 commits November 15, 2021 11:18

Improves robustness of primal::closest_point() operator via fuzzy com…

9a77173

…parisons Using new `EPS` tolerance parameter.

Updates data submodule

3eac867

Fixes compilation errors with nvcc

5209b20

Initializes input parameter in signed distance example

802e56f

Includes some additional minor fixes.

Adds a counter to the SignedDistance MinCandidate structure

4018b3f

This counter tracks the number of entities at the current closest point. This is expected to be 2 for edges and higher for vertices. Added per PR suggestion.

Tightens tolerance for closest point computations in SignedDistance q…

feb296e

…uery

Bugfix for wrong case in SignedDistance's angle-weighted norm calcula…

3a8b811

…tion

Refactors how the quest regression tests are defined in the build system

1fb92c9

Adds some additional quest regression tests

aa7c1dd

Adds tests for the `tetrahedron` mesh as well as the `boxedSphere` mesh. Also adds a regression configuration for the `plane_simp` mesh, as described in #703

Adds a missing operator that gets a vector by subtracting two points

1e0ab97

Updates data submodule for new STL meshes and regression data

9cc3ff7

Updates RELEASE-NOTES

00ca410

Enables extra quest regression tests in 'linux_clang10' Azure pipelin…

3ff4c48

…e job

Allow linux-build_and_test script to take a BUILD_TYPE variable (…

204af0c

…default: Debug) Also use `Release` build for `linux_clang10` Azure CI job.

Adds an option to build_src script to build with a given CMake `bui…

0a94a2e

…ld_type` Options are: 'Debug' (default), 'RelWithDebInfo', 'Release' and 'RelMinSize'

Adds a new gitlab job for a release build of clang@10 with extra qu…

756eb03

…est regression tests

Fixes a typo

8821ea2

kennyweiss force-pushed the bugfix/kweiss/signed-distance branch from cb2457b to 8821ea2 Compare November 15, 2021 19:19

agcapps approved these changes Nov 15, 2021

View reviewed changes

publixsubfan approved these changes Nov 15, 2021

View reviewed changes

rhornung67 approved these changes Nov 15, 2021

View reviewed changes

kennyweiss merged commit 11aafb3 into develop Nov 15, 2021

kennyweiss deleted the bugfix/kweiss/signed-distance branch November 15, 2021 21:19

kennyweiss mentioned this pull request Feb 5, 2022

Improve robustness of SignedDistance computation (redux) #783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves robustness of `SignedDistance` query #706

Improves robustness of `SignedDistance` query #706

kennyweiss commented Nov 8, 2021 •

edited

Loading

rhornung67 Nov 8, 2021

publixsubfan Nov 8, 2021

kennyweiss Nov 8, 2021

kennyweiss Nov 8, 2021

kennyweiss Nov 8, 2021

bmhan12 Nov 10, 2021

kennyweiss Nov 10, 2021 •

edited

Loading

kennyweiss commented Nov 10, 2021

agcapps left a comment

agcapps left a comment

publixsubfan left a comment

publixsubfan Nov 15, 2021

	cmake_dependent_option( AXOM_MINT_USE_SIDRE "Enables Sidre support in Mint" ON
	"AXOM_ENABLE_SIDRE" OFF)

Improves robustness of SignedDistance query #706

Improves robustness of SignedDistance query #706

Conversation

kennyweiss commented Nov 8, 2021 • edited Loading

Summary

TODO

rhornung67 Nov 8, 2021

Choose a reason for hiding this comment

publixsubfan Nov 8, 2021

Choose a reason for hiding this comment

kennyweiss Nov 8, 2021

Choose a reason for hiding this comment

kennyweiss Nov 8, 2021

Choose a reason for hiding this comment

kennyweiss Nov 8, 2021

Choose a reason for hiding this comment

bmhan12 Nov 10, 2021

Choose a reason for hiding this comment

kennyweiss Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

kennyweiss commented Nov 10, 2021

agcapps left a comment

Choose a reason for hiding this comment

agcapps left a comment

Choose a reason for hiding this comment

publixsubfan left a comment

Choose a reason for hiding this comment

publixsubfan Nov 15, 2021

Choose a reason for hiding this comment

Improves robustness of `SignedDistance` query #706

Improves robustness of `SignedDistance` query #706

kennyweiss commented Nov 8, 2021 •

edited

Loading

kennyweiss Nov 10, 2021 •

edited

Loading