Fix for overwriting nodesets #4121

DanielYankura · 2025-03-27T22:29:29Z

When creating sidesets from nodesets if the sideset shares an id with a nodeset they are combined
This checks first that the two are actually the same (same id and same nodes)
If they are not the same the instead of overwriting a new id is assigned to the new nodeset
If this is to be merged some tests in MOOSE would have to be re-golded

Should be merged after idaholab/moose#30291

- When creating sidesets from nodesets if the sideset shares an id with a nodeset they are combined - This checks first that the two are actually the same (same id and same nodes) - If they are not the same the instead of overwriting a new id is assigned to the new nodeset refs idaholab/moose#30085

- May need minor adjustments to work for when users manually rename sideset IDs

- Removed commented out code - Changed Map from nodeset to sideset id to a set of boundary IDs - Added function that allows MOOSE to add an id to this set

moosebuild · 2025-04-11T17:52:03Z

Job Coverage, step Generate coverage on ca63b3e wanted to post the following:

Coverage

	5063fb	#4121 ca63b3
	Total	Total	+/-	New
Rate	63.41%	63.41%	+0.00%	97.44%
Hits	74790	74814	+24	38
Misses	43153	43162	+9	1

Diff coverage report

Full coverage report

This comment will be updated on new commits.

DanielYankura · 2025-04-22T14:45:07Z

I checked the tests that failed and it looks like it was because it was tested on those three MOOSE tests I fixed in idaholab/moose#30291 before the PR made it to next. I invalidated them to run them again. Hopefully that should fix it.

roystgnr · 2025-04-23T14:14:49Z

meshmodifiers/element_subdomain_modifier.boundary/moving_boundary FAILED (TIMEOUT) [FINISHED]

A timeout, so probably random, but I'm going to kick it again to be safe.

roystgnr · 2025-04-23T14:15:30Z

And of course the MOOSE ARM mac failure is no more than the same civet breakage; we'll ignore that.

DanielYankura · 2025-04-23T14:50:35Z

That's what I figured. Those two tests weren't failing when I first opened the PR and they aren't failing when I run them locally.

roystgnr

I think we need unit tests here, maybe before even bothering to rewrite the code that will address them. Come up with cases that should throw an error, and cases that shouldn't, and add a little test using each.

roystgnr · 2025-04-23T14:38:21Z

include/mesh/boundary_info.h

@@ -386,6 +386,14 @@ class BoundaryInfo : public ParallelObject
   */
  void renumber_id (boundary_id_type old_id, boundary_id_type new_id);

+  /**
+   * Checks for existing nodeset that matches with this sideset


We're going to need much more explanation here. What does "matches" mean? I'd have personally implemented this as return false;, because nodesets and sidesets are two conceptually different things and thus any return true; would be a category error. But since MOOSE has dived into that error too deep to fix, we're going to have to at least explain the new mishmashed category.

roystgnr · 2025-04-23T14:40:17Z

src/mesh/boundary_info.C

+  ElemSideBuilder side_builder;
+  const Elem * new_side = &side_builder(*elem, side);
+  if (has_equivalent_nodeset(new_side, id))
+    _sideset_to_nodeset_conversion.insert(id);


If I give a side BCID 5, and then later I add an unrelated node with BCID 5, are those equivalent? They're definitely not, but here at the side addition we're already insisting that they must be.

roystgnr · 2025-04-23T14:45:27Z

src/mesh/boundary_info.C

+  const auto * n_list = side->get_nodes();
+  for (unsigned int i = 0; i < side->n_nodes(); i++)
+  {
+    const auto node = n_list[i];


This is not a good way to iterate over an element's nodes. Try node_ref_range().

roystgnr · 2025-04-23T14:50:16Z

src/mesh/boundary_info.C

+      }
+      else
+        equivalent_nodeset = false;
+    }


This loop doesn't make sense to me. If I have sides a and b with nodes A, B, and C, A--a--B--b--C, and I have BCID 3 on a, b, and A, would you say that sideset 3 is equivalent in any way to nodeset 3? I'd say it's definitely not, and unless I'm misunderstanding the code here has_equivalent_nodeset(b,3) would agree with me, but has_equivalent_nodeset(a,3) would disagree?

I honestly don't know what concept we're attempting to compute here.

roystgnr · 2025-04-23T14:52:46Z

src/mesh/boundary_info.C

          // Add each node node on the side with the side's boundary id
          for (auto i : side->node_index_range())
+          {
+            const boundary_id_type bcid = id_pair.second;
+            auto bcid_renum = bcid;


We're renumbering the bcid? But only ever to the same number? What is this variable for?

roystgnr · 2025-04-23T14:55:24Z

src/mesh/boundary_info.C

+            const boundary_id_type bcid = id_pair.second;
+            auto bcid_renum = bcid;
+            // If grouping by name check if nodeset id needs to be renumbered
+            if (_node_boundary_ids.find(bcid) != _node_boundary_ids.end() && !has_equivalent_nodeset(side, bcid))


If nothing about this test changes from node to node, then we don't want to call it inside the side->node_index_range() loop.

roystgnr · 2025-04-23T15:01:02Z

src/mesh/boundary_info.C

+            // If grouping by name check if nodeset id needs to be renumbered
+            if (_node_boundary_ids.find(bcid) != _node_boundary_ids.end() && !has_equivalent_nodeset(side, bcid))
+              // This sideset overlaps with a nodeset. Throw an error
+              libmesh_error_msg("Sideset " << bcid << " has the same ID as a preexisting nodeset. Rename one in order to build a nodeset from this sideset");


I love this error checking in spirit. In practice we're not going to call it nearly as often as we should, right?

Consider again A--a--B--b--C, with BCID 5 on node A and on sides a and b.

Suppose our element loop hits side a first.

We try to build_node_list_from_side_list(). On side a, has_equivalent_nodeset() returns true because it sees node A, so it happily adds BCID 5 to node B. Then on side b, has_equivalent_nodeset() returns true because it sees node B, so it happily adds BCID 5 to node C, and it finishes, having avoided emitting an error that IMHO it definitely should have.

Now suppose our loop takes us to b before a. On b, has_equivalent_nodeset() doesn't see any of the BCID 5 nodes that it knows exist, so it returns false, and now we emit the error.

But our results shouldn't depend on element numbering.

DanielYankura marked this pull request as draft March 27, 2025 22:29

DanielYankura force-pushed the overwriting_existing_nodeset branch 2 times, most recently from 98c714a to 1015024 Compare April 9, 2025 22:57

DanielYankura mentioned this pull request Apr 10, 2025

Changes several tests so that they still pass with new libmesh feature idaholab/moose#30291

Merged

DanielYankura marked this pull request as ready for review April 10, 2025 14:35

DanielYankura added 4 commits April 10, 2025 08:45

Now throws an error instead of attempting to renumber nodesets

f595806

- May need minor adjustments to work for when users manually rename sideset IDs

Minor changes

80d4e32

- Removed commented out code - Changed Map from nodeset to sideset id to a set of boundary IDs - Added function that allows MOOSE to add an id to this set

Deleted trailing whitespace

4ee955c

DanielYankura force-pushed the overwriting_existing_nodeset branch from 1015024 to 4ee955c Compare April 10, 2025 14:55

removed more trailing whitespace

ca63b3e

roystgnr requested changes Apr 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix for overwriting nodesets #4121

Fix for overwriting nodesets #4121

Uh oh!

DanielYankura commented Mar 27, 2025 •

edited

Loading

Uh oh!

moosebuild commented Apr 11, 2025

Uh oh!

DanielYankura commented Apr 22, 2025

Uh oh!

roystgnr commented Apr 23, 2025

Uh oh!

roystgnr commented Apr 23, 2025

Uh oh!

DanielYankura commented Apr 23, 2025

Uh oh!

roystgnr left a comment

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

roystgnr Apr 23, 2025

Uh oh!

Uh oh!

Fix for overwriting nodesets #4121

Are you sure you want to change the base?

Fix for overwriting nodesets #4121

Uh oh!

Conversation

DanielYankura commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moosebuild commented Apr 11, 2025

Coverage

Uh oh!

DanielYankura commented Apr 22, 2025

Uh oh!

roystgnr commented Apr 23, 2025

Uh oh!

roystgnr commented Apr 23, 2025

Uh oh!

DanielYankura commented Apr 23, 2025

Uh oh!

roystgnr left a comment

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

roystgnr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielYankura commented Mar 27, 2025 •

edited

Loading