DG parallelization #1415

lihanyu97 · 2025-06-24T19:08:07Z

This is work in progress to parallelize DG scheme relying MFEM's parallel communication infrastructure (ExchangeFaceNbrData). The first phase of this work to configure residual computation in parallel is complete. The main effort is to properly prepare input data that includes ghost dofs for Functional and record the correct index in element restriction to ensure the evaluation and AD at each quadrature point accesses the right ghost dof value. The comment associated with each major implementation is included.

Please point out anything that is obviously wrong or clumsy, and I will avoid them in the next phase.

tupek2

So, maybe I'm going dense... but where is the test that you get the same field solution on both sides of the dg interface for pre-specified linear/quadratic fields?

Looks like amazing progress. Are you intending to push this in incrementally, or wait until the jacobian is done?

src/serac/numerics/functional/element_restriction.cpp

src/serac/numerics/functional/functional.hpp

src/serac/numerics/functional/tests/dg_restriction_operators.cpp

src/serac/numerics/functional/tests/functional_basic_dg.cpp

lihanyu97 · 2025-06-26T21:32:29Z

So, maybe I'm going dense... but where is the test that you get the same field solution on both sides of the dg interface for pre-specified linear/quadratic fields?

Looks like amazing progress. Are you intending to push this in incrementally, or wait until the jacobian is done?

The new test is here: https://github.com/LLNL/serac/blob/42804174f653ba821530ff9d09210a401aa283e6/src/serac/numerics/functional/tests/dg_ghost_face_index.cpp

I would say let's push everything in after jacobian is fixed bacause this PR is making some fundamental changes on the typedef.
mfem::Mesh --> mfem::ParMesh
mfem::FiniteElementSpace --> mfem::ParFiniteElementSpace

lihanyu97 · 2025-07-13T05:33:09Z

@tupek2 @btalamini This DG PR is ready for review. All debug footprints (std::out / mpi::out) are removed.

src/serac/numerics/functional/tests/dg_ghost_face_index.cpp

src/serac/numerics/functional/tests/domain_tests.cpp

src/serac/numerics/functional/typedefs.hpp

src/serac/numerics/functional/domain.cpp

btalamini · 2025-07-30T23:37:44Z

src/serac/numerics/functional/domain.hpp

@@ -96,6 +96,9 @@ struct Domain {
   */
  std::map<FunctionSpace, BlockElementRestriction> restriction_operators;

+  /// @brief Shared interior face list


This comment just repeats the variable name, and so isn't useful. Think of a user who has never seen this class before and is reading these. What does "shared" mean in this context? Tell them what this is for. Here's a suggestion:

/// @brief Ids of interior faces that lie on the boundary shared by two processors

But feel free to change this if you can make it clearer.

btalamini · 2025-08-01T19:57:12Z

src/serac/numerics/functional/domain.hpp

+   * @brief Find the list of interior faces shared by two processors to make sure
+   *        these faces are only integrated once.
+   */
+  void insert_shared_interior_face_list();


I think this function should be considered logically private, and therefore be called inside InteriorFaces(...) (and any other factory function - maybe ofEdges?) . In my opinion, if one calls InteriorFaces(...) and the resulting Domain has an empty shared_interior_face_ids_, it's in an invalid state. The user shouldn't need to know to call an additional method for the class to be usable in parallel. @lihanyu97 and @tupek2, do you agree?

btalamini · 2025-08-01T19:59:49Z

src/serac/numerics/functional/domain.cpp

+  SLIC_ERROR_IF(type_ != Domain::Type::InteriorFaces, "This method is only for interior face domains");
+
+  // make a list if we don't already have one
+  if (shared_interior_face_ids_.size() == 0) {


I prefer this, as the intent is clearer:

Suggested change

if (shared_interior_face_ids_.size() == 0) {

if (shared_interior_face_ids_.empty()) {

src/serac/numerics/functional/element_restriction.cpp

btalamini · 2025-08-01T20:39:53Z

src/serac/numerics/functional/functional_qoi.inl

    }

+    domain.insert_shared_interior_face_list();


As noted above, I think the shared face list should be filled on the domain construction and not be the responsibility of the user here.

The reason I implemented this way is because 1) shared interior face list is only used in qoi functionals, and 2) if the qoi involves integrating over a set of interior faces. This means if we always fill this interior_face_ids_ container for all InteriorFace domains, we are requesting additional memory that we may never use. Therefore, I only require this container to be filled automatically from AddInteriorFaceIntegral in functional_qoi.inl. The user doesn't have to worry about filling this container. You can see that AddInteriorFaceIntegral in functional.hpp doesn't have this line.

Also, this method is similar to insert_restriction, which doesn't do anything if there's already an element restriction for a given trial space. So for example, if we our functional qoi is computed by adding two different interior face integrals, we need to call AddInteriorFaceIntegral twice. But insert_shared_interior_face_list only execute once.

I understand. But I think the decision of whether to allocate that memory is made when the user decides to instantiate a Domain of interior faces. The point of the domains is to integrate over them, and the shared face information is needed to compute integrals on this type of domain. If it doesn't have it, it can't do its job in all cases. I would view that as having an invalid state.

"Users" in this case are not just people making Functional objects, they're also developers using the Domain class. If someone writes another integrator - like the dFEM one, or example - it won't be obvious to them that they need to call an additional method on interior face domains to be able to use the shared face info.

I can see why the restriction operators lead you to this. But it turns out that this is not a good model. They were tacked onto Domain as an afterthought. We might pull them back out. I don't want to replicate that pattern.

Really, the underlying problem is that Domain should be a class with constructors, data hiding, and a clear contract. That's not your fault. We'll address it later.

btalamini · 2025-08-01T22:54:20Z

src/serac/numerics/functional/functional.hpp

+ * @brief helper function to locally cast away const on FE space to update face neighbor data.
+ *  this is ok because the original FE space is declared without const and we constrained
+ *  the non-constness locally


I don't understand this comment. What I hope it means is that this function exists because the MakeRef method in MFEM is not const correct, but we know and trust that it does not actually change the trial space. If my statement is wrong, we need to revisit this.

MakeRef is not the problem. ExchangeFaceNbrData is the method that doesn't have const keyword because it creates/recreates the FaceNbrData vector and other infos related to the face neighbor data. The catch here is MakeRef links the ParGridFunction with external data, so it doesn't create any additional memory footprint. Now if the external data is const, then we cannot invoke ExchangeFaceNbrData on the ParGridFunction.

So what I meant in the comment is that, we can cast away the const locally in this method because we want to invoke ExchangeFaceNbrData. This is ok because we are not changing any of the locally owned data on the trial space. All we are changing/updating is the face neighbor data.

btalamini · 2025-08-01T23:18:51Z

src/serac/numerics/functional/domain.cpp

+void Domain::insert_shared_interior_face_list()
+{
+  // Weights only need to be computed for Domain of InteriorFaces type
+  SLIC_ERROR_IF(type_ != Domain::Type::InteriorFaces, "This method is only for interior face domains");


I think in this case you should use SLIC_ERROR_ROOT_IF to avoid echoing the output on all ranks.

btalamini · 2025-08-01T23:36:40Z

src/serac/numerics/functional/tests/dg_ghost_face_index.cpp

+using namespace serac::profiling;
+
+template <int dim, int p>
+void L2_index_test(std::string meshfile)


I would appreciate if you write a short comment block that explains the intent of the test. If it ever fails, it will help the person who broke it understand what the test's job is and hopefully give a clue as to how they broke it. As a group, we do not usually do this, so this is a hypocritical complaint.

Sure I will add comments to this test.

lihanyu97 added 4 commits June 5, 2025 17:19

serial dg works with new structure to support parallelization

7b37012

Configured DG ghost data and indexing. Added test

b0e4bc1

Fixed three tests

ccf8329

Fixed output size

6d088cf

lihanyu97 requested review from white238, ebchin, btalamini and tupek2 June 24, 2025 19:08

lihanyu97 added 2 commits June 24, 2025 21:17

Fixed true residual size

abf5111

Refactor ghost data reordering. Fix ActionOfGradient

4280417

tupek2 reviewed Jun 26, 2025

View reviewed changes

lihanyu97 added 4 commits July 9, 2025 16:03

fixed assemble, basic test passed in parallel

f7a0b94

dg parallelized

b9c5958

update shared interior face list

eb535bb

Merge branch 'develop' into li/DGparallel_Hdiv

3136f3f

lihanyu97 marked this pull request as ready for review July 13, 2025 04:53

lihanyu97 changed the title ~~WIP: DG parallelization~~ DG parallelization Jul 13, 2025

lihanyu97 added the ready for review Ready for active inspection by reviewers label Jul 13, 2025

white238 reviewed Jul 14, 2025

View reviewed changes

src/serac/numerics/functional/tests/dg_ghost_face_index.cpp Outdated Show resolved Hide resolved

white238 reviewed Jul 16, 2025

View reviewed changes

src/serac/numerics/functional/tests/dg_ghost_face_index.cpp Outdated Show resolved Hide resolved

white238 reviewed Jul 16, 2025

View reviewed changes

src/serac/numerics/functional/tests/domain_tests.cpp Show resolved Hide resolved

white238 reviewed Jul 16, 2025

View reviewed changes

src/serac/numerics/functional/typedefs.hpp Outdated Show resolved Hide resolved

Add suggested clean up

3cfd828

tupek2 approved these changes Jul 17, 2025

View reviewed changes

src/serac/numerics/functional/domain.cpp Show resolved Hide resolved

Merge branch 'develop' into li/DGparallel_Hdiv

e3cd5e0

btalamini requested changes Aug 1, 2025

View reviewed changes

	if (shared_interior_face_ids_.size() == 0) {
	if (shared_interior_face_ids_.empty()) {

DG parallelization #1415

Are you sure you want to change the base?

DG parallelization #1415

Uh oh!

Conversation

lihanyu97 commented Jun 24, 2025

Uh oh!

tupek2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lihanyu97 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lihanyu97 commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihanyu97 Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihanyu97 Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihanyu97 Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lihanyu97 commented Jun 26, 2025 •

edited

Loading

lihanyu97 commented Jul 13, 2025 •

edited

Loading

lihanyu97 Aug 2, 2025 •

edited

Loading

lihanyu97 Aug 2, 2025 •

edited

Loading

lihanyu97 Aug 2, 2025 •

edited

Loading