Skip to content
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.

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

Support Blueprint mesh in shaping #1455

Open
wants to merge 124 commits into
base: develop
Choose a base branch
from

Conversation

gunney1
Copy link
Contributor

@gunney1 gunney1 commented Oct 21, 2024

Motivation

IntersectionShaper currently support only MFEM mesh. That was due to convenience when the class was developed. IntersectionShaper doesn't make any use of any features not supported by Blueprint. We need to support Blueprint mesh, as that is the standard for LLNL.

Summary

  • This PR is a feature
  • It is a follow-up to and depends on PR Construct shapes in memory and support more shapes #1436. It is kept separate to minimize the size of the change set.
  • It addresses issue Support Blueprint mesh in IntersectionShaper and the shaper in-memory shape tests #1452
  • It does the following:
    • Adds Blueprint mesh support in IntersectionShaper. This largely consists of
      • Constructing with Blueprint mesh objects, as either conduit::Node or sidre::Group.
      • Replacing mfem::GridFunction* with axom::ArrayView<double> as a means of handling cell-centered data.
    • Support incoming mesh data on device or on host and require that the data lives in a space that's compatible with the selected runtime execution policy.
    • Move some long pieces of code into functions, to improve readability.
    • Move allocation and computation of mesh-dependent data into a function to de-clutter, isolate and optimize out the one-time overhead cost. The cost runs high for device memory and obscures other performance bottlenecks.
    • Extends an example (quest_shape_in_memory.cpp) to test the Blueprint mesh support.

Planned follow-up.

To keep this PR and its companion as as small as possible, I avoided moving code around too much. As a result the code is a bit messy. Issue #1445 tracks the planned follow-on work.

gunney1 added 30 commits October 2, 2024 17:13
This version compiles without MFEM, if conduit is supported.
But it doesn't run because the conduit side is not fully written yet.
GridFunctionView is renamed TempArrayView to reflect that it doesn't
work with just GridFunctions.
…memory' into feature/gunney/blueprint-mesh-shaping
The feature was never used and we are moving away from MFEM mesh
in favor of blueprint mesh.
Add blueprint support to shaper code and test code.
The code required what to Blueprint was optional.
Facilitated by ability to reshape array data in sidre Views.
Also add code to save results for visualization.
Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me. I defer to others more familiar with this code regarding details of the changes.

@gunney1 gunney1 force-pushed the feature/gunney/blueprint-mesh-shaping branch from 364b7b2 to 97b531c Compare January 27, 2025 16:19
src/axom/core/execution/runtime_policy.hpp Show resolved Hide resolved
src/axom/core/execution/execution_space.hpp Outdated Show resolved Hide resolved
src/axom/klee/tests/klee_shape_set.cpp Show resolved Hide resolved
src/axom/quest/DiscreteShape.cpp Show resolved Hide resolved
src/axom/quest/Shaper.cpp Outdated Show resolved Hide resolved
src/axom/quest/util/mesh_helpers.hpp Outdated Show resolved Hide resolved
, m_bpNodeExt(nullptr)
, m_bpNodeInt()
#endif
, m_comm(MPI_COMM_WORLD)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe outside the scope of this PR but in the CMakeLists.txt, there are some files that are only enabled when AXOM_USE_MPI is enabled. This file is always enabled and it contains the m_comm member. Can quest build without MPI? It looked like before the communicator was mostly accessed through the MFEM data collection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quest can build without MPI -- but it uses a fake comm in those cases (which is apparently hidden in an obscure location :) )

See: https://github.com/LLNL/axom/blob/develop/src/axom/quest/interface/internal/mpicomm_wrapper.hpp

(As I recall, this was seen as a one-off at the time, and @white238 was hoping to work on an MPI wrapper component.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't know about the MPI_Comm wrapper.

auto order = mapping.getStrideOrder();
if(int(order) & int(axom::ArrayStrideOrder::COLUMN))
{
RAJA::kernel<EXEC_POL>(
Copy link
Member

@BradWhitlock BradWhitlock Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we need variadic axom::for_all() - or overloads.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gunney1

  • The Shaper classes are getting pretty complicated, which indicates that we probably need to redesign them to support all the cases we care about in a cleaner way. Since you're planning follow-up PRs, I'm ok with this happening in the future, but we should think this through before it gets too complicated.
  • Also, can you please explain why Umpire is needed for all shapers, even if we're only using the CPU? Is there a way to use Axom's memory abstraction to avoid this requirement?
  • There are a lot of small utility functions that are added/modified throughout. Please consider adding unit tests for these, as appropriate
  • One cleanup note: sidre always depends on conduit, so if we have a file that uses sidre without checking if AXOM_USE_SIDRE, then it is safe to assume that conduit is available without guards.

src/axom/core/CMakeLists.txt Show resolved Hide resolved
src/axom/core/execution/internal/seq_exec.hpp Outdated Show resolved Hide resolved
src/axom/core/utilities/FileUtilities.cpp Show resolved Hide resolved
src/axom/core/utilities/FileUtilities.cpp Show resolved Hide resolved
src/axom/klee/Geometry.cpp Outdated Show resolved Hide resolved
src/axom/quest/Shaper.hpp Show resolved Hide resolved
src/axom/quest/Shaper.hpp Show resolved Hide resolved
src/axom/quest/Shaper.hpp Outdated Show resolved Hide resolved
src/axom/quest/examples/shaping_driver.cpp Show resolved Hide resolved
src/axom/quest/examples/shaping_driver.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Quest Issues related to Axom's 'quest' component User Request Issues related to user requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Blueprint mesh format in IntersectionShaper
8 participants