Fix of loading boundary elements of ParMesh from files #4128

najlkin · 2024-02-09T23:33:08Z

This PR fixes the problem recognized in #4083 . When ParMesh is loaded from files/streams through its constructor or Load() method with refine or fix_orientation parameters set true (as by default), boundary elements are generated for every boundary face including the shared ones if the local mesh does not have by chance at least one boundary element already loaded from the file, in which case the global boundary is preserved and no boundary elements are generated. This is a completely inconsistent behavior causing spurious appearance of internal boundaries depending on partitioning of the ParMesh.

The problem is/was in method Mesh::GenerateBoundaryElements(), which does not check anyhow if the boundary is shared or not. Here, it is overridden in ParMesh and no boundary elements are generated on shared faces.

PR	Author	Editor	Reviewers	Assignment	Approval	Merge
#4128	@najlkin	@tzanio	@sebastiangrimberg + @sohailreddy + @jandrej	2/10/24	4/12/24	4/15/24

PR Checklist

mesh/pmesh.cpp

tzanio · 2024-02-11T02:27:28Z

This PR is now under review (see the table in the PR description). To help with the review process, please do not force push to the branch.

sebastiangrimberg · 2024-02-12T18:26:37Z

This is a good bug fix and resolves the issue in #4083. I would suggest also including the following change to mesh.cpp as part of this PR to avoid calling GenerateBoundaryElements in parallel situations when it is unintended:

diff --git a/mesh/mesh.cpp b/mesh/mesh.cpp
index ce8dc7cf5..b830c499b 100644
--- a/mesh/mesh.cpp
+++ b/mesh/mesh.cpp
@@ -3014,7 +3014,7 @@ void Mesh::FinalizeTetMesh(int generate_edges, int refine, bool fix_orientation)
    FinalizeCheck();
    CheckElementOrientation(fix_orientation);

-   if (NumOfBdrElements == 0)
+   if (ReduceInt(NumOfBdrElements) == 0)
    {
       GetElementToFaceTable();
       GenerateFaces();
@@ -3056,7 +3056,7 @@ void Mesh::FinalizeWedgeMesh(int generate_edges, int refine,
    FinalizeCheck();
    CheckElementOrientation(fix_orientation);

-   if (NumOfBdrElements == 0)
+   if (ReduceInt(NumOfBdrElements) == 0)
    {
       GetElementToFaceTable();
       GenerateFaces();
@@ -3093,7 +3093,7 @@ void Mesh::FinalizeHexMesh(int generate_edges, int refine, bool fix_orientation)
    GetElementToFaceTable();
    GenerateFaces();

-   if (NumOfBdrElements == 0)
+   if (ReduceInt(NumOfBdrElements) == 0)
    {
       GenerateBoundaryElements();
    }
@@ -3166,7 +3166,7 @@ void Mesh::FinalizeTopology(bool generate_bdr)
    {
       GetElementToFaceTable();
       GenerateFaces();
-      if (NumOfBdrElements == 0 && generate_bdr)
+      if (ReduceInt(NumOfBdrElements) == 0 && generate_bdr)
       {
          GenerateBoundaryElements();
          GetElementToFaceTable(); // update be_to_face
@@ -3186,7 +3186,7 @@ void Mesh::FinalizeTopology(bool generate_bdr)
       if (Dim == 2)
       {
          GenerateFaces(); // 'Faces' in 2D refers to the edges
-         if (NumOfBdrElements == 0 && generate_bdr)
+         if (ReduceInt(NumOfBdrElements) == 0 && generate_bdr)
          {
             GenerateBoundaryElements();
          }
@@ -3200,7 +3200,7 @@ void Mesh::FinalizeTopology(bool generate_bdr)
    if (Dim == 1)
    {
       GenerateFaces();
-      if (NumOfBdrElements == 0 && generate_bdr)
+      if (ReduceInt(NumOfBdrElements) == 0 && generate_bdr)
       {
          // be_to_face will be set inside GenerateBoundaryElements
          GenerateBoundaryElements();
@@ -3264,7 +3264,7 @@ void Mesh::Finalize(bool refine, bool fix_orientation)
    }
    if (refine)
    {
-      MarkForRefinement();   // may change topology!
+      MarkForRefinement(); // may change topology!
    }

    if (may_change_topology)
@@ -3277,7 +3277,8 @@ void Mesh::Finalize(bool refine, bool fix_orientation)
       }
       else
       {
-         FinalizeTopology(); // Re-computes some data unnecessarily.
+         bool generate_bdr = false;
+         FinalizeTopology(generate_bdr); // re-computes some data unnecessarily
       }

       // TODO: maybe introduce Mesh::NODE_REORDER operation and FESpace::

najlkin · 2024-02-12T19:31:19Z

I am not sure about bool generate_bdr = false;. It is an intended behavior perhaps 🤔

sebastiangrimberg · 2024-02-12T19:34:35Z

I am not sure about bool generate_bdr = false;. It is an intended behavior perhaps 🤔

Because Finalize already assumes FinalizeTopology has been called, there are two options:

Either FinalizeTopology was called with generate_bdr = true, in which case calling it here with generate_bdr = false has no effect
FinalizeTopology was called with generate_bdr = false, in which case calling it here with generate_bdr = true as is currently the behavior on master is producing unintended consequences for the user

So, I think in either of these cases adding the explicit generate_bdr = false argument is not going to introduce unwelcome changes. But the other reviewers should chime in.

najlkin · 2024-02-27T17:46:25Z

To move things forward a little bit, @jandrej , @sohailreddy , what are your opinions on generate_bdr = false? Note that ParMesh::Finalize() is a public method and Mesh::Finalize() generates boundaries automatically (when refine or fix_orientation are true).

najlkin · 2024-04-09T22:25:07Z

Yes, that is where we have started 😄 . I edited the header of #4240 to mention it avoids this as well 😉 . Please keep this PR minimal, so it can get finally merged. The overhead is only in the loader, so practically negligible, and it is not caused by this PR.

sebastiangrimberg · 2024-04-09T22:41:24Z

Yes, that is where we have started 😄 . I edited the header of #4240 to mention it avoids this as well 😉 . Please keep this PR minimal, so it can get finally merged. The overhead is only in the loader, so practically negligible and not caused by this PR.

Just need to reemphasize that my suggestion to remove the boundary element generation in the first part of this comment would be one way of remove that limitation and make the fixes in this PR even more minimal. I don't think there was clear consensus that this was a necessary feature to add though maybe I am missing something.

Another option would be to use the approach suggested in this comment and avoid accessing sedge_ledge or sface_lface inside of ParMesh::GenerateBoundaryElements. You can get the same information from the (already constructed) shared_edges, shared_trias, and shared_quads for your boundary element generation.

najlkin · 2024-04-09T23:13:38Z

Just need to reemphasize that my suggestion to remove the boundary element generation in the first part of #4128 (comment) would be one way of remove that limitation and make the fixes in this PR even more minimal. I don't think there was clear consensus that this was a necessary feature to add though maybe I am missing something.

This bugfix is not adding a feature, but recovers the intended behavior, which is consistent with the behavior of serial meshes. I believe majority is on the side of keeping things consistent, or at least for a bugfix. We may return to this discussion in another PR if you want to open it for this.

Another option would be to use the approach suggested in #4128 (comment) and avoid accessing sedge_ledge or sface_lface inside of ParMesh::GenerateBoundaryElements. You can get the same information from the (already constructed) shared_edges, shared_trias, and shared_quads for your boundary element generation.

If we do not want to go beyond the bugfix and touch calls of FinalizeParTopo(), there is no need for the change. We would be just risking breaking the implementation. However, it is a good suggestion, just I am not convinced it is worth of it, but we may discuss it within #4240 😉 .

sebastiangrimberg · 2024-04-09T23:20:01Z

OK, I've voiced my concerns but can only defer to editors/other reviewers at this point.

v-dobrev · 2024-04-10T02:29:50Z

@najlkin, I think the decision at the meeting was to NOT include the parallel boundary generation but just make the checks (NumOfBdrElements == 0) global and leave the parallel version of GenerateBoundaryElements empty, at least for now.

This way we postpone the controversial changes for further discussion but get the important bugfix in.

najlkin · 2024-04-10T13:43:54Z

@v-dobrev , I thought the decision was to split this PR, as I did, moving the potentially controversial change of the initialization of some structures to #4240 . The implementation of Mesh::GenerateBoundaryElements() in ParMesh is not controversial in the sense we all believe it is working correctly. It has been tested on the code of the user and the user also confirmed it is working for him in #4083 . @sebastiangrimberg is proposing two other things:

The alternative of removing the functionality of generating boundaries all together from ParMesh.
Cleaning up the pipeline of (Par)Mesh, which goes along the lines of Cleaner initialization of shared structures in ParMesh #4240 , but he would go even further.

Both are API changes and should be discussed in #4240 , but not here (!). Please someone just confirm this is working, so it can go to v4.7 asap 🙏 .

v-dobrev · 2024-04-10T20:17:04Z

I'll let @tzanio handle this -- he's the editor.

najlkin · 2024-04-10T20:36:44Z

@jandrej , have you tested this PR is working? That is all we need now, the discussion related to the API changes should go to #4240 .

tzanio · 2024-04-11T20:55:14Z

As a compromise: is it just possible to just ParMesh::GenerateBoundaryElements = delete which will explicitly expose this as a bug to the users?

@sebastiangrimberg, @v-dobrev and @najlkin -- what do you think?

sebastiangrimberg · 2024-04-11T22:43:46Z

As a compromise: is it just possible to just ParMesh::GenerateBoundaryElements = delete which will explicitly expose this as a bug to the users?

@sebastiangrimberg, @v-dobrev and @najlkin -- what do you think?

I like this idea but am not sure you can do that in C++ if I understand correctly:

./mfem/linalg/../fem/../mesh/pmesh.hpp:402:9: error: deleted function 'GenerateBoundaryElements' cannot override a non-deleted function
  402 |    void GenerateBoundaryElements() override = delete;

Leaving it defined as {} or as { MFEM_ABORT(" ..."); } would achieve similar effect at runtime (either silently or not)?

najlkin · 2024-04-11T22:45:54Z

@sebastiangrimberg , it must be without virtual (and override)

sebastiangrimberg · 2024-04-11T22:49:36Z

@sebastiangrimberg , it must be without virtual (and override)

Oh I see, but doesn't that mean you could get this behavior for a valid ParMesh?

ParMesh pmesh(...);
pmesh.GenerateBoundaryElements();  // Compiler error
Mesh *mesh = &pmesh;
mesh->GenerateBoundaryElements();  // Not a compiler error, bug-prone?

najlkin · 2024-04-11T22:56:59Z

I think that behavior is correct in the sense that when you explicitly use the local sub-mesh, you can do many harmful things to the distributed mesh for sure, but that is responsibility of the user and might be desired for some master hacker 🤓

sebastiangrimberg · 2024-04-11T23:00:50Z

I think it means that when Mesh::FinalizeTopology eventually gets called from ParMesh::Load, you will get a call to Mesh::GenerateBoundaryElements (not ParMesh::GenerateBoundaryElements, which is correctly deleted) which is what I thought was what we wanted to avoid.

tzanio · 2024-04-11T23:17:01Z

OK, then I vote to implement it as { MFEM_ABORT(" ..."); } and note that users shouldn't be calling that function.

najlkin · 2024-04-11T23:17:11Z

Hmm, well, it is not critical, because that happens only when you do not have any boundary elements globally, which was not the case for the user and most other users. Anyway, I think a working implementation would be a better option, but to follow this compromise we may make FinalizeTopology() virtual and call the serial implementation with generate_bdr=false from ParMesh.

najlkin · 2024-04-11T23:18:41Z

...or abort, if generate_bdr==true

…Elements()" This reverts commit 44c2e22.

This reverts commit 1a1a6fe.

najlkin · 2024-04-12T00:10:44Z

Ok, so followed the pathway of @tzanio 's comment 😉 .

tzanio · 2024-04-13T05:40:55Z

Merged in next for testing...

najlkin added 2 commits February 9, 2024 13:28

Fixed loading of boundary elements in ParMesh.

1a1a6fe

Improved the local-shared search in ParMesh::GenerateBoundaryElements()

44c2e22

najlkin added bug mesh hpc labels Feb 9, 2024

najlkin self-assigned this Feb 9, 2024

najlkin mentioned this pull request Feb 9, 2024

Boundary corruption when loading ParMesh from files #4083

Closed

sebastiangrimberg reviewed Feb 9, 2024

View reviewed changes

mesh/pmesh.cpp Outdated Show resolved Hide resolved

sebastiangrimberg reviewed Feb 10, 2024

View reviewed changes

mesh/pmesh.cpp Outdated Show resolved Hide resolved

najlkin added the ready-for-review label Feb 10, 2024

tzanio added in-review and removed ready-for-review labels Feb 11, 2024

tzanio assigned jandrej, sebastiangrimberg, sohailreddy and tzanio Feb 11, 2024

tzanio requested review from jandrej and sohailreddy February 11, 2024 02:27

tzanio added this to Review Now in Pull Requests via automation Feb 11, 2024

tzanio added this to the mfem-4.7 milestone Feb 11, 2024

najlkin added 2 commits February 12, 2024 12:55

Moved initialization of the shared-local maps to the source.

24a7358

Fixed the check of boundary elements.

8a3ffed

This was referenced Feb 27, 2024

Marking / Deleting internal boundaries #4147

Closed

Bdr attributes with ParSubMesh #4145

Closed

tzanio mentioned this pull request Mar 4, 2024

Final changes for mfem-4.7 #4172

Merged

46 tasks

najlkin added 3 commits April 11, 2024 16:27

Revert "Improved the local-shared search in ParMesh::GenerateBoundary…

e8aea98

…Elements()" This reverts commit 44c2e22.

Revert "Fixed loading of boundary elements in ParMesh."

5427a92

This reverts commit 1a1a6fe.

Prohibited usage of GenerateBoundaryElements() for ParMesh.

b1d0bfe

sebastiangrimberg approved these changes Apr 12, 2024

View reviewed changes

najlkin mentioned this pull request Apr 12, 2024

Implementation of ParMesh::GenerateBoundaryElements() #4244

Open

tzanio approved these changes Apr 13, 2024

View reviewed changes

tzanio added the in-next label Apr 13, 2024

tzanio added a commit that referenced this pull request Apr 13, 2024

Merge branch 'najlkin/pmesh-load-fix' into next (PR #4128)

5cf3063

tzanio merged commit 12ed616 into master Apr 15, 2024
27 checks passed

Pull Requests automation moved this from Review Now to Merged Apr 15, 2024

tzanio deleted the najlkin/pmesh-load-fix branch April 15, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix of loading boundary elements of ParMesh from files #4128

Fix of loading boundary elements of ParMesh from files #4128

najlkin commented Feb 9, 2024 •

edited by tzanio

tzanio commented Feb 11, 2024

sebastiangrimberg commented Feb 12, 2024

najlkin commented Feb 12, 2024

sebastiangrimberg commented Feb 12, 2024

najlkin commented Feb 27, 2024 •

edited

najlkin commented Apr 9, 2024 •

edited

sebastiangrimberg commented Apr 9, 2024 •

edited

najlkin commented Apr 9, 2024

sebastiangrimberg commented Apr 9, 2024

v-dobrev commented Apr 10, 2024 •

edited

najlkin commented Apr 10, 2024 •

edited

v-dobrev commented Apr 10, 2024

najlkin commented Apr 10, 2024

tzanio commented Apr 11, 2024 •

edited

sebastiangrimberg commented Apr 11, 2024 •

edited

najlkin commented Apr 11, 2024

sebastiangrimberg commented Apr 11, 2024

najlkin commented Apr 11, 2024

sebastiangrimberg commented Apr 11, 2024 •

edited

tzanio commented Apr 11, 2024

najlkin commented Apr 11, 2024

najlkin commented Apr 11, 2024

najlkin commented Apr 12, 2024 •

edited

tzanio commented Apr 13, 2024

Fix of loading boundary elements of ParMesh from files #4128

Fix of loading boundary elements of ParMesh from files #4128

Conversation

najlkin commented Feb 9, 2024 • edited by tzanio

tzanio commented Feb 11, 2024

sebastiangrimberg commented Feb 12, 2024

najlkin commented Feb 12, 2024

sebastiangrimberg commented Feb 12, 2024

najlkin commented Feb 27, 2024 • edited

najlkin commented Apr 9, 2024 • edited

sebastiangrimberg commented Apr 9, 2024 • edited

najlkin commented Apr 9, 2024

sebastiangrimberg commented Apr 9, 2024

v-dobrev commented Apr 10, 2024 • edited

najlkin commented Apr 10, 2024 • edited

v-dobrev commented Apr 10, 2024

najlkin commented Apr 10, 2024

tzanio commented Apr 11, 2024 • edited

sebastiangrimberg commented Apr 11, 2024 • edited

najlkin commented Apr 11, 2024

sebastiangrimberg commented Apr 11, 2024

najlkin commented Apr 11, 2024

sebastiangrimberg commented Apr 11, 2024 • edited

tzanio commented Apr 11, 2024

najlkin commented Apr 11, 2024

najlkin commented Apr 11, 2024

najlkin commented Apr 12, 2024 • edited

tzanio commented Apr 13, 2024

najlkin commented Feb 9, 2024 •

edited by tzanio

najlkin commented Feb 27, 2024 •

edited

najlkin commented Apr 9, 2024 •

edited

sebastiangrimberg commented Apr 9, 2024 •

edited

v-dobrev commented Apr 10, 2024 •

edited

najlkin commented Apr 10, 2024 •

edited

tzanio commented Apr 11, 2024 •

edited

sebastiangrimberg commented Apr 11, 2024 •

edited

sebastiangrimberg commented Apr 11, 2024 •

edited

najlkin commented Apr 12, 2024 •

edited