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.

Sign up for GitHub

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

Jump to bottom

Add Pro/E input to intersection shaping driver #1111

Merged

bmhan12 merged 33 commits into develop from feature/han12/shaping_proe

Jun 13, 2023

Contributor

bmhan12 commented Jun 6, 2023 •

edited

Loading

This PR:

Adds Pro/E format as an input to the intersection shaping driver
Updates data submodule with klee and Pro/E files to test the new format in the quest_intersection_shaper unit tests:

Related to LLNL/axom_data#16

Pro/E Case 1:

Pro/E Case 2:

bmhan12 added Quest Primal labels

bmhan12 requested review from BradWhitlock, kennyweiss, publixsubfan and agcapps

June 6, 2023 18:08

bmhan12 mentioned this pull request

Pro/E Unit Test Files LLNL/axom_data#16

Merged

kennyweiss approved these changes

View reviewed changes

Member

kennyweiss left a comment

Thanks @bmhan12!

Please don't forget to update the RELEASE-NOTES.

src/axom/primal/geometry/Octahedron.hpp Show resolved Hide resolved

src/axom/primal/geometry/Polyhedron.hpp Outdated

Comment on lines 610 to 631

+                  for(int i = 0; i < m_num_vertices; i++)
+                  {
+                    for(int j = i + 1; j < m_num_vertices; j++)
+                    {
+                      // operator= for Point does not want to play nice...
+                      if(m_vertices[i][0] == m_vertices[j][0] &&
+                         m_vertices[i][1] == m_vertices[j][1] &&
+                         m_vertices[i][2] == m_vertices[j][2])
+                      {
+                        return false;
+                      }
+                    }
+                  }

Member

kennyweiss Jun 6, 2023

@bmhan12 -- do we always want to call a Polyhedron invalid when it has duplicated vertices?
There are some cases that I can think of where this might be beneficial.

Also, should we be checking for exact duplicates? or should we be supplying a tolerance and checking that the distance b/w vertices is not nearly zero subject to that tolerance?

Suggestion: I think it would be clearer to move the highlighted code to a new function hasDuplicatedVertices(double EPS), and consider calling it explicitly, when necessary.

Contributor Author

bmhan12 Jun 6, 2023

do we always want to call a Polyhedron invalid when it has duplicated vertices?

Admittedly, I'm not sure.
I did find this case needs to be handled for the Polyhedron::volume() function. It resulted in an infinite loop condition.
I've added a new test in primal_clip.cpp for this case in particular.

Also, should we be checking for exact duplicates? or should we be supplying a tolerance and checking that the distance b/w vertices is not nearly zero subject to that tolerance?

👍 . Agreed, a tolerance value would be good.

Suggestion: I think it would be clearer to move the highlighted code to a new function hasDuplicatedVertices(double EPS), and consider calling it explicitly, when necessary.

I can separate this snippet into the function, and have it just in the Polyhedron::volume() function for now where I was originally seeing the problem.

src/axom/quest/IntersectionShaper.hpp Outdated Show resolved Hide resolved

src/axom/quest/IntersectionShaper.hpp Outdated

Comment on lines 393 to 406

+                  // Initialize tetrahedra
+                  axom::Array<IndexType> nodeIds(4);
+                  axom::Array<Point3D> pts(4);
-                  const auto& shapeName = shape.getName();
-                  AXOM_UNUSED_VAR(shapeDimension);
-                  AXOM_UNUSED_VAR(shapeName);
+                  for(int i = 0; i < m_tetcount; i++)
+                  {
+                    m_surfaceMesh->getCellNodeIDs(i, nodeIds.data());
-                  SLIC_INFO(axom::fmt::format("Current shape is {}", shapeName));
+                    m_surfaceMesh->getNode(nodeIds[0], pts[0].data());
+                    m_surfaceMesh->getNode(nodeIds[1], pts[1].data());
+                    m_surfaceMesh->getNode(nodeIds[2], pts[2].data());
+                    m_surfaceMesh->getNode(nodeIds[3], pts[3].data());
+                    m_tets[i] = TetrahedronType({pts[0], pts[1], pts[2], pts[3]});
+                  }

Member

kennyweiss Jun 6, 2023

Is there a way to do this w/in a kernel in the ExecSpace too?

Contributor Author

bmhan12 Jun 6, 2023

Is there a way to do this w/in a kernel in the ExecSpace too?

I did not explore it in this PR, as a lot of these calls are to mint::UnstructuredMesh functions which are not host/device annotated yet.
The functions also override virtual mint::Mesh functions, so that might complicate things.

src/axom/quest/IntersectionShaper.hpp Outdated Show resolved Hide resolved

src/axom/quest/IntersectionShaper.hpp Outdated Show resolved Hide resolved

src/axom/quest/IntersectionShaper.hpp Outdated Show resolved Hide resolved

src/axom/quest/IntersectionShaper.hpp Outdated

		@@ -882,11 +1005,12 @@ class IntersectionShaper : public Shaper
		axom::deallocate(m_hexes);

Member

kennyweiss Jun 6, 2023

I'm a bit confused about when we use allocate/deallocate and when we use StackArray for variables/arrays.
Could you please clarify?

Member

kennyweiss Jun 6, 2023

(Perhaps that's why I have so many comments about it)

Contributor Author

bmhan12 Jun 6, 2023 •

edited

Loading

I'm a bit confused about when we use allocate/deallocate and when we use StackArray for variables/arrays.
Could you please clarify?

The StackArray's that are being created here are for using the host-device decorated ArrayView constructor:

axom/src/axom/core/ArrayView.hpp

Lines 76 to 87 in 31ae2e4

    
             /*! 
        
              * \brief Generic constructor for an ArrayView of arbitrary dimension with external data 
        
              * 
        
              * \param [in] data the external data this ArrayView will wrap. 
        
              * \param [in] shape Array size in each dimension. 
        
              * \param [in] spacing_ Spacing between consecutive items. 
        
              * 
        
              * This constructor supports non-unit spacing. 
        
              */ 
        
             AXOM_HOST_DEVICE ArrayView(T* data, 
        
                                        const StackArray<IndexType, DIM>& shape, 
        
                                        IndexType spacing_ = 1);

The allocate/deallocate is for memory that will be used inside kernels.

I need to do a pass through these to change them to use axom::Array and axom::ArrayView.

src/axom/quest/IntersectionShaper.hpp Outdated Show resolved Hide resolved

agcapps approved these changes

View reviewed changes

Member

agcapps left a comment

Thanks, Brian!

rhornung67 approved these changes

View reviewed changes

Member

rhornung67 left a comment

Thanks @bmhan12 A few comments for consideration related to documentation and implementation consistency

src/axom/primal/geometry/Octahedron.hpp Outdated Show resolved Hide resolved

src/axom/primal/geometry/Tetrahedron.hpp Show resolved Hide resolved

src/axom/primal/operators/split.hpp Outdated Show resolved Hide resolved

src/axom/primal/tests/primal_clip.cpp Outdated Show resolved Hide resolved

src/axom/quest/interface/internal/QuestHelpers.hpp Outdated Show resolved Hide resolved

bmhan12 added 20 commits

June 12, 2023 15:07


          Add read_pro_e_mesh() to QuestHelpers

9568fb8


          Test read_pro_e_mesh() usage --not sure if I will keep this?

e534268


          Update submodule with examples

6b10c22


          Add Pro/E option to loadShape()

81ff869


          WIP - separate out Pro/E and c2c workflow

8e832c4


          sanity Pro/E mesh output

fc283fb


          Add compute bounding box overload for tetrahedron

daafa84


          WIP - setup tet mesh

253d72d


          Post merge changes - filterMesh() operation for c2c guard

a3def93


          WIP - runShapeQuery equivalent for proe

d8caa1d


          IndexType over int

48882cd


          Check file format and file type before reading

100fce7


          Check for duplicate vertices in valid polyhedron check; add special t…

93fe2fe

…est case for clipping


          WIP - move format check inside IntersectionShaper class instead of at…

66813c3

… the driver level


          Add mixed proe/c2c unit test case

129b16f


          Add proe-only test case with baseline

0eb779d


          Properly guard the new proe tests in non-RAJA, non-Umpire case

fd260c0


          Refactoring of prepareShapeQuery

67c9f8c


          Use ArrayView over pointer copies

eda9dcc


          Refactor runShapeQuery; make NUM_VERTS standard between Octs and Tets

4ce4d77

bmhan12 added 13 commits

June 12, 2023 15:09


          Cleanup

917ed8f


          Put duplicate vertices check into kernel body to remove warning; bugf…

f42abd0

…ix wrong count


          Unused

f263fd1


          Make hasDuplicateVertices() function standalone with tolerance, remov…

50d2d43

…e duplicate logic in driver


          Convert bounding boxes and hexes to Array

811a52b


          Additional Array refactoring, but hanging on rzvernal...

c843ec6


          Leave troublesome allocations as ptrs instead of Arrays

72bfd43


          Refactor oct/tet members to use Array, refactor Discretize functions …

7cb0d8a

…to use Array to accomodate changes


          Workaround changes for gcc@8.1.0 compiler

d340e93


          Update notes

b2a7b1b


          Wordsmithing

d799f4d


          smithing, consistent style

f4c1907


          Workaround for marching cubes for gcc@8.1.0~fortran compiler

dc5bd9e

bmhan12 force-pushed the feature/han12/shaping_proe branch from 375b17f to dc5bd9e Compare

June 13, 2023 16:03

Contributor Author

bmhan12 commented Jun 13, 2023

The conversion of pointer allocate/deallocate memory to axom:Array / axom::ArrayView was mostly straightforward.

Did encounter an error with HIP trying to use axom::Array with a RAJA::ReduceSum for the size:

axom/src/axom/quest/IntersectionShaper.hpp

Lines 729 to 755 in 43df7b6

    
           // Get the total number of candidates 
        
           using REDUCE_POL = typename axom::execution_space<ExecSpace>::reduce_policy; 
        
           using ATOMIC_POL = typename axom::execution_space<ExecSpace>::atomic_policy; 
        
           const auto counts_v = counts.view(); 
        
           RAJA::ReduceSum<REDUCE_POL, int> totalCandidates(0); 
        
           axom::for_all<ExecSpace>( 
        
             NE, 
        
             AXOM_LAMBDA(axom::IndexType i) { totalCandidates += counts_v[i]; }); 
        
           // Initialize hexahedron indices and octahedra candidates 
        
           axom::IndexType* hexIndices = 
        
             axom::allocate<axom::IndexType>(totalCandidates.get() * NUM_TETS_PER_HEX); 
        
           axom::IndexType* octCandidates = 
        
             axom::allocate<axom::IndexType>(totalCandidates.get() * NUM_TETS_PER_HEX); 
        
           // Tetrahedrons from hexes (24 for each hex) 
        
           TetrahedronType* tets = 
        
             axom::allocate<TetrahedronType>(NE * NUM_TETS_PER_HEX); 
        
           // Index into 'tets' 
        
           axom::IndexType* tetIndices = 
        
             axom::allocate<axom::IndexType>(totalCandidates.get() * NUM_TETS_PER_HEX); 
        
           // New total number of candidates after omitting degenerate octahedra 
        
           int* newTotalCandidates = axom::allocate<int>(1); 
        
           axom::copy(newTotalCandidates, ZERO, sizeof(int));

Not seeing the problem with CUDA.

This case will be added to a running list of HIP issues in #787 .

bmhan12 mentioned this pull request

Remaining Issues with HIP #787

Open

8 tasks

bmhan12 merged commit d467ebe into develop

bmhan12 deleted the feature/han12/shaping_proe branch

June 13, 2023 17:00

bmhan12 mentioned this pull request

Fix mesh tester example and add tests #1142

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kennyweiss kennyweiss approved these changes

rhornung67 rhornung67 approved these changes

agcapps agcapps approved these changes

BradWhitlock Awaiting requested review from BradWhitlock

publixsubfan Awaiting requested review from publixsubfan

Labels