Implement first set of fast approximation GWN methods#1791
Conversation
| public: | ||
| using NURBSCurve = axom::primal::NURBSCurve<double, 2>; | ||
| using CurveArrayView = axom::ArrayView<NURBSCurve>; | ||
| using CurveArrayView = axom::ArrayView<const NURBSCurve>; |
There was a problem hiding this comment.
I had to make this change for this to work more straight-forwardly with LinearizeCurves, since otherwise (to my knowledge) you can't use it to linearize a view of const objects without making a copy of all of them. But I think this is more close to how the class is actually implemented everywhere else, right? It doesn't seem to me like there's anywhere that a LinearizeCurves object ever needs to change a NURBSCurve in it's view.
9142cea to
4609f08
Compare
4609f08 to
cfd9c5c
Compare
kennyweiss
left a comment
There was a problem hiding this comment.
Partial review: first batch of comments
| } | ||
| else | ||
| { | ||
| // Do it in 2 stages. |
There was a problem hiding this comment.
Can you please expand on the two steps and why we need them for the non-seq policy?
Looks like step one is to compute on the leaf nodes and step 2 is to move it up the tree.
(whereas the seq policy branch does both)
There was a problem hiding this comment.
I think the main benefit is just to improve the efficiency of operating over all the leaves, and I'm not certain that needs to be an exclusively parallel thing. I could probably rewrite reduce_recursion to not even take a lambda function, and just pass a view to the leaf array. I think @BradWhitlock originally wrote this, do you know of any particular reason it can't be done in two stages for both policies?
There was a problem hiding this comment.
@jcs15c -- is there a simple example or unit test we can add to spin for the new reduce_tree traverser? E.g. adding a random collection of points to a BVH by their bounding boxes and the counting the number of points contained within each inner node?
There was a problem hiding this comment.
Yes, I'm sure I can come up with something. One way that's agnostic to the way the tree is constructed is that, if you have 8 points added to the BVH, then 8 inner nodes should contain one point (the leaves), 4 contain two points, 2 contain four points, and 1 contains all eight points, right?
kennyweiss
left a comment
There was a problem hiding this comment.
Thanks @jcs15c -- this looks great! I have a bunch of minor comments; nothing substantial.
| std::string pjoin(const char *str, Args... args) | ||
| { | ||
| return axom::utilities::filesystem::joinPath(std::string(str), pjoin(args...)); | ||
| } |
There was a problem hiding this comment.
@BradWhitlock -- should we move these pjoin functions to core/utilities? Perhaps in a separate PR?
| * the collection of geometric objects. | ||
| */ | ||
| template <typename T, int NDIMS, int ORD> | ||
| class GWNMomentData |
There was a problem hiding this comment.
Would it make sense to add some unit tests for the moment data computations? Or is this covered by the segment/triangle tests?
There was a problem hiding this comment.
Added some for 2D segments and 3D triangles!
| template <typename ExecSpace, typename ValueType, typename LeafAction> | ||
| axom::Array<ValueType> reduce_tree(LeafAction&& lf, | ||
| int allocatorID = axom::getDefaultAllocatorID()) const | ||
| { |
There was a problem hiding this comment.
Is the eventual plan for this function to be invokable from inside a GPU kernel as a device function? If not, I'd suggest moving it into the standard BVH interface.
There was a problem hiding this comment.
We'd like to, but are not there yet
There was a problem hiding this comment.
As I understand it, a method like this couldn't go directly into the standard BVH interface because only LinearBVH describes how the data at internal nodes is actually arranged into arrays and arrayviews. This output of this function needs to align with that internal layout, and so if there was ever another implementation type for BVH, the actual logic of the function would have to change to reflect the internal layout of that other implementation. Does that seem consistent with how BVH.hpp and LinearBVH.hpp relate to one another?
9993c59 to
0f31be3
Compare
bc6e432 to
3f33b76
Compare
| poly_mesh, | ||
| AXOM_LAMBDA(axom::IndexType cellIdx, | ||
| const axom::numerics::Matrix<double>& coords, | ||
| [[maybe_unused]] const axom::IndexType* nodeIds) { |
There was a problem hiding this comment.
hip is not happy w/ this attribute. Looks like we should change it to something like
| [[maybe_unused]] const axom::IndexType* nodeIds) { | |
| const axom::IndexType* /*nodeIds*/) { |
... and similarly for the one below
Arlie-Capps
left a comment
There was a problem hiding this comment.
@jcs15c , thank you for your hard work!
| string(CONCAT _pass_regex | ||
| "INOUT_STATS:.*" | ||
| "pos_dofs=16.*" | ||
| "pos_dofs=14.*" |
There was a problem hiding this comment.
Just for future reference, the new OCC-derived AABB method has slightly different bounds, so there are different numbers of contained points.
Summary
quest_winding_number_2d_ex(previouslyquest_winding_number_ex) andquest_winding_number_3d_exby adding options for fast approximations of the GWN field. These methods are currently implemented for collections of 3D triangles and 2D lines as described in this paper.quest_winding_number_3d_exexample now has command line arguments to triangulate the input STEP file and evaluate its GWN field, andquest_winding_number_2d_exnow has command line arguments to linearize input curves. Subsequent containment decisions will faithfully reproduce the boundary of the discrete shape, but this will not exactly match the boundary of the curved surface. This leads to classification errors for points close to the surface.Algorithm Details
Implementation Details
LinearBVHTraverser::reduce_tree(<LeafAction>)method which evaluates theLeafActionlambda on each leaf node to produce a value, then adding those values up the tree so that each internal node is associated with the sum of values for all its children. This returns an array of these values for each internal node.This PR should consider the following before being un-drafted:Done!quest_winding_number_3d_exto take in.stlfiles directly as input.