Fix mesh tester example and add tests #1142

bmhan12 · 2023-07-19T22:52:33Z

This PR:

Fixes failing mesh_tester executable
- Bug was introduced by a change made in PR Add Pro/E input to intersection shaping driver #1111 (link to exact buggy commit)
Adds tests for mesh_tester for the different spatial indexes and RAJA policies.
Re-enable core_openmp_map unit test.
- AXOM_USE_RAJA was used of instead of RAJA_FOUND to guard test inclusion.

…a policies

rhornung67

agcapps · 2023-07-20T17:04:20Z

Thanks for the fixes, @bmhan12 . I see you test RAJA_FOUND. I think we may also want to test && AXOM_USE_RAJA to allow a user to manually turn off RAJA for Axom even though it's there. Thoughts?

agcapps

Approved, but perhaps we also want to test against AXOM_USE_RAJA.

rhornung67 · 2023-07-20T17:13:07Z

@agcapps doesn't RAJA_FOUND imply that AXOM_USE_RAJA is true/defined?

bmhan12 · 2023-07-20T17:24:10Z

I think we may also want to test && AXOM_USE_RAJA to allow a user to manually turn off RAJA for Axom even though it's there. Thoughts?

Hm, this might make things confusing, at least in terms of separation of defines for source code (AXOM_USE_XXX), and ones for CMake (XXX_FOUND , ENABLE_XXX, AXOM_ENABLE_XXX).

In these cases AXOM_USE_RAJA is not currently defined for CMake, so the checks fail.

bmhan12 · 2023-07-20T17:28:30Z

doesn't RAJA_FOUND imply that AXOM_USE_RAJA is true/defined?

Yes, relevant CMake configuration:

axom/src/cmake/AxomConfig.cmake

Lines 13 to 21 in e809258

    
           ## Add a definition to the generated config file for each library dependency 
        
           ## (optional and built-in) that we might need to know about in the code. We 
        
           ## check for vars of the form <DEP>_FOUND or ENABLE_<DEP> 
        
           set(TPL_DEPS C2C CAMP CLI11 CONDUIT CUDA FMT HIP HDF5 LUA MFEM MPI OPENMP RAJA SCR SOL SPARSEHASH UMPIRE ) 
        
           foreach(dep ${TPL_DEPS}) 
        
               if( ${dep}_FOUND OR ENABLE_${dep} ) 
        
                   set(AXOM_USE_${dep} TRUE  ) 
        
               endif() 
        
           endforeach()

rhornung67 · 2023-07-20T17:41:06Z

@bmhan12 I looks like we use AXOM_USE_RAJA in the source code, but RAJA_FOUND in the CMakeLists.txt files. So I think what you have done makes sense. @agcapps do you agree? We should confirm with @white238

agcapps · 2023-07-20T17:42:24Z

@agcapps doesn't RAJA_FOUND imply that AXOM_USE_RAJA is true/defined?

OK, so in https://github.com/LLNL/axom/blob/develop/src/cmake/AxomConfig.cmake#L16-L21, AXOM_USE_RAJA gets set if RAJA_FOUND is true or the user passed in ENABLE_RAJA. Looking at source code, header and source files seem to test AXOM_USE_RAJA, but CMakeLists.txt files (which is what you're working on, Brian) seem to test RAJA_FOUND.

Looks like you are doing the right thing. Sorry for the noise.

rhornung67 · 2023-07-20T17:46:37Z

An occasional round of confusion is cleansing.... 😄

kennyweiss

Thanks @bmhan12 !

kennyweiss · 2023-07-21T00:49:17Z

src/axom/quest/detail/Discretize_detail.hpp

    axom::for_all<ExecSpace>(
      curr_lvl_count,
      AXOM_LAMBDA(axom::IndexType i) {
-        out[next_lvl + i * lvl_factor + 0] =
-          new_inscribed_prism(out[curr_lvl + i], Q, T, S, R, pa, pb);
-        out[next_lvl + i * lvl_factor + 1] =
-          new_inscribed_prism(out[curr_lvl + i], U, R, Q, P, pa, pb);
+        OctType *out_ptr = out.data();
+        out_ptr[next_lvl + i * lvl_factor + 0] =
+          new_inscribed_prism(out_ptr[curr_lvl + i], Q, T, S, R, pa, pb);
+        out_ptr[next_lvl + i * lvl_factor + 1] =
+          new_inscribed_prism(out_ptr[curr_lvl + i], U, R, Q, P, pa, pb);
        if(level == 0)
        {
-          out[next_lvl + i * lvl_factor + 2] =
-            new_inscribed_prism(out[curr_lvl + i], S, P, U, T, pa, pb);
+          out_ptr[next_lvl + i * lvl_factor + 2] =
+            new_inscribed_prism(out_ptr[curr_lvl + i], S, P, U, T, pa, pb);
        }
      });


I'm surprised it's necessary to grab a pointer to out, via out.ptr() before changing values in the array.
That's not how we'd want to have to use ArrayView.

Would it help if we add the mutable keyword? E.g.

AXOM_LAMBDA(axom::IndexType i) mutable { out[ <some_index> ] = new_inscribed_prism(out[ <other_index> ], <params> ); ... }

We do that in a few places in axom, e.g.:

axom/src/axom/spin/tests/spin_bvh.cpp

Line 1404 in e809258

AXOM_LAMBDA(std::int32_t idx) mutable {

axom/src/axom/quest/DistributedClosestPoint.hpp

Line 1262 in e809258

AXOM_LAMBDA(std::int32_t idx) mutable {

@kennyweiss Thanks! mutable works here!

kennyweiss · 2023-07-21T00:50:12Z

src/tools/CMakeLists.txt

+    # Add unit tests
+    if(AXOM_ENABLE_TESTS AND AXOM_DATA_DIR AND RAJA_FOUND AND UMPIRE_FOUND)
+
+        # Run the mesh_tester with the plane_simp_problems.stl example and
+        # different spatial indexes, raja policies
+
+        set(plane_data_dir ${AXOM_DATA_DIR}/quest/plane_simp_problems.stl)
+
+        set(_methods "bvh" "implicit" "uniform")
+
+        set (_policies "raja_seq")
+        blt_list_append(TO _policies ELEMENTS "raja_omp" IF AXOM_ENABLE_OPENMP)
+        blt_list_append(TO _policies ELEMENTS "raja_cuda" IF AXOM_ENABLE_CUDA)
+
+        foreach(_method ${_methods})
+            foreach(_policy ${_policies})
+
+                set(_testname "mesh_tester_${_method}_${_policy}")
+                axom_add_test(
+                  NAME ${_testname}
+                  COMMAND mesh_tester
+                          --infile ${plane_data_dir}
+                          --method ${_method}
+                          --policy ${_policy}
+                )
+
+                set_tests_properties(${_testname} PROPERTIES
+                    PASS_REGULAR_EXPRESSION  "5 intersecting tri pairs")
+            endforeach()
+        endforeach()
+    endif()


src/tools/CMakeLists.txt

Co-authored-by: Chris White <white238@llnl.gov>

bmhan12 added 3 commits July 19, 2023 14:04

Fix for mesh_tester failures

86afd3a

Better fix to gcc@8.1.0 const discard qualifier error

cd6de61

Re-enable core_openmp_map test (guarded by wrong RAJA CMake option)

2379820

bmhan12 added the Testing Issues related to testing Axom label Jul 19, 2023

bmhan12 added 2 commits July 19, 2023 16:01

Add unit tests for mesh_tester with different spatial indexes and raj…

9f0a18c

…a policies

Include Umpire in guard

f1421f2

bmhan12 force-pushed the bugfix/han12/debug_mesh_tester branch from c1e8492 to f1421f2 Compare July 19, 2023 23:01

rhornung67 approved these changes Jul 20, 2023

View reviewed changes

bmhan12 requested review from white238, kennyweiss, agcapps and publixsubfan July 20, 2023 15:29

agcapps approved these changes Jul 20, 2023

View reviewed changes

kennyweiss approved these changes Jul 21, 2023

View reviewed changes

Better fix - make lambda mutable to fix const qualifier error

6a956b2

white238 reviewed Jul 21, 2023

View reviewed changes

src/tools/CMakeLists.txt Outdated Show resolved Hide resolved

white238 approved these changes Jul 21, 2023

View reviewed changes

Add quotation marks for Windows case

7cf73e5

Co-authored-by: Chris White <white238@llnl.gov>

bmhan12 merged commit a3000a3 into develop Jul 21, 2023
10 checks passed

bmhan12 deleted the bugfix/han12/debug_mesh_tester branch July 21, 2023 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mesh tester example and add tests #1142

Fix mesh tester example and add tests #1142

bmhan12 commented Jul 19, 2023

rhornung67 left a comment

agcapps commented Jul 20, 2023

agcapps left a comment

rhornung67 commented Jul 20, 2023

bmhan12 commented Jul 20, 2023

bmhan12 commented Jul 20, 2023

rhornung67 commented Jul 20, 2023

agcapps commented Jul 20, 2023

rhornung67 commented Jul 20, 2023 •

edited

Loading

kennyweiss left a comment

kennyweiss Jul 21, 2023 •

edited

Loading

bmhan12 Jul 21, 2023

kennyweiss Jul 21, 2023

Fix mesh tester example and add tests #1142

Fix mesh tester example and add tests #1142

Conversation

bmhan12 commented Jul 19, 2023

rhornung67 left a comment

Choose a reason for hiding this comment

agcapps commented Jul 20, 2023

agcapps left a comment

Choose a reason for hiding this comment

rhornung67 commented Jul 20, 2023

bmhan12 commented Jul 20, 2023

bmhan12 commented Jul 20, 2023

rhornung67 commented Jul 20, 2023

agcapps commented Jul 20, 2023

rhornung67 commented Jul 20, 2023 • edited Loading

kennyweiss left a comment

Choose a reason for hiding this comment

kennyweiss Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

bmhan12 Jul 21, 2023

Choose a reason for hiding this comment

kennyweiss Jul 21, 2023

Choose a reason for hiding this comment

rhornung67 commented Jul 20, 2023 •

edited

Loading

kennyweiss Jul 21, 2023 •

edited

Loading