Add vacuum and reflective BCs using the MoabSkinner by meltawila · Pull Request #1286 · neams-th-coe/cardinal

meltawila · 2026-02-04T17:12:15Z

closes #1285

moosebuild · 2026-02-04T17:33:36Z

Job Documentation, step Sync to remote on 585a259 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild · 2026-02-04T18:22:49Z

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

Coverage

	9d0111	#1286 585a25
	Total	Total	+/-	New
Rate	93.16%	93.21%	+0.05%	100.00%
Hits	9779	9889	+110	114
Misses	718	720	+2	0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

…orrectly

moosebuild · 2026-03-24T03:38:03Z

Job Precheck, step Clang format on 7b6eb7b wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/cardinal/docs/PRs/1286/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 9d01119411a3ede62b36e8d0952b0358083f866d

meltawila · 2026-03-24T18:46:17Z

@aprilnovak this should be ready for review now

aprilnovak · 2026-03-26T00:34:52Z

+  params.addParam<std::vector<BoundaryName>>("vacuum_bcs_surfaces",
+                                             "Sideset names/ids to assign vacuum BCs to");
+  params.addParam<std::vector<BoundaryName>>("reflective_bcs_surfaces",
+                                             "Sideset names/ids to assign reflective BCs to");


Question: does the code work with either names or IDs?

aprilnovak · 2026-03-26T00:37:07Z

+{
+  auto namesToSidesetIDs =
+      [&](const std::vector<BoundaryName> & names, const std::string & param_name)
+  {


this inline function seems more complicated than it needs to be? it does not use the param_name input. Since you just call this right below, I'd suggest to get rid of this inline thing to simplify the code.

aprilnovak · 2026-03-26T00:37:57Z

+    {
+      const BoundaryID id = getMooseMesh().getBoundaryID(n);
+      if (!getMooseMesh().meshSidesetIds().count(id))
+        mooseError("Boundary '", n, "' is not a sideset.");


Does this work properly if user provides IDs directly in the .i file?

aprilnovak · 2026-03-26T00:38:27Z

+  for (auto id : vac_ids)
+    if (ref_ids.count(id))
+      mooseError(
+          "Same sideset ", id, " appears in both vacuum_bcs_surfaces and reflective_bcs_surfaces.");


Is this covered by a test?

aprilnovak · 2026-03-26T00:39:34Z

+  for (const auto * elem : getMooseMesh().getMesh().active_element_ptr_range())
+  {
+    auto elem_to_moab_tets_it = _id_to_elem_handles.find(elem->id());
+    if (elem_to_moab_tets_it == _id_to_elem_handles.end())


does this ever get called or is this dead code?

aprilnovak · 2026-03-26T00:47:49Z

+  {
+    const moab::EntityHandle surf_set = surf_pair.first;
+    moab::Range tris;
+    check(_moab->get_entities_by_handle(surf_set, tris));


what is this check checking?

aprilnovak · 2026-03-26T00:47:53Z

+        (!ref_tris.empty() && !moab::intersect(tris, ref_tris).empty());
+
+    if (has_vacuum_tris)
+      check(_moab->add_entities(vac_group, &surf_set, 1));


what is this check checking?

aprilnovak · 2026-03-26T00:47:56Z

+    if (has_vacuum_tris)
+      check(_moab->add_entities(vac_group, &surf_set, 1));
+    if (has_reflective_tris)
+      check(_moab->add_entities(ref_group, &surf_set, 1));


what is this check checking?

aprilnovak · 2026-03-26T00:48:58Z

+    input = openmc.i
+    csvdiff = 'all_vacuum.csv'
+    cli_args = 'UserObjects/moab/vacuum_bcs_surfaces="top bottom front back left right" Outputs/file/file_base="all_vacuum"'
+    requirement = "The system shall give identical Monte Carlo solution (file mesh tallies and k) when adding "


the requirement needs to be unique for each test. This is identical to the all_reflective test.

and several other tests here, it looks like

aprilnovak · 2026-03-26T00:49:35Z

+  []
+
+  allow_renumbering = false
+  parallel_type = replicated


Since you already have the restriction to replicated mesh in the tests file, you don't need to include it here

gonuke

A few questions/comments based on a first pass through this. I'll try to take a deeper look soon.

Also, while I'm not fully versed in the style conventions of this project, it seems like the function to add the boundary conditions would be more readable if broken into a number of smaller functions each with a clear semantic purpose.

gonuke · 2026-03-26T11:56:44Z

+  for (auto id : vac_ids)
+    if (ref_ids.count(id))
+      mooseError(
+          "Same sideset ", id, " appears in both vacuum_bcs_surfaces and reflective_bcs_surfaces.");


Don't you want to test this sooner, ie. in the constructor?

gonuke · 2026-03-26T11:57:36Z

+  const auto ref_ids = namesToSidesetIDs(_reflective_bcs_surfaces, "reflective_bcs_surfaces");
+
+  for (auto id : vac_ids)
+    if (ref_ids.count(id))


Is there a simple way to use set logic to get all the overlapping ids at once and issue a single error with them all?

gonuke · 2026-03-26T12:00:05Z

+      for (const auto tet : elem_to_moab_tets_it->second)
+      {
+        moab::Range tri_faces;
+        check(_moab->get_adjacencies(&tet, 1, 2, true, tri_faces));


Could also replace those magic numbers (1,2) with a variable that clarifies their purpose.

gonuke · 2026-03-26T12:07:48Z

 }

+void
+MoabSkinner::addBoundaryConditionGroups()


I wonder if this would be simpler if the method only handled one of the boundary conditions at a time, and was called once for each boundary conditions (assuming that the user has requested it)

gonuke · 2026-03-26T12:16:14Z

+
+  const auto & boundary_info = getMooseMesh().getMesh().get_boundary_info();
+
+  for (const auto * elem : getMooseMesh().getMesh().active_element_ptr_range())


(Caveat: I'm not deeply familiar with this particular implementation of skinning, but am very familiar with the concept.) This whole loop appears to be almost like it's performing a skinning operation again. Could some of this information have been captured more easily during the process of skinning the mesh rather than going back after the fact to query the mesh again?

gonuke · 2026-03-26T12:22:49Z

+      const moab::EntityHandle moab_surface_meshset = surface_entry.first;
+      unsigned int surface_id = 0;
+      check(_moab->tag_get_data(id_tag, &moab_surface_meshset, 1, &surface_id));
+      max_surface_id = std::max(max_surface_id, surface_id);


I wonder if it's more efficient to load all the surface meshset handles into a vector and get all the IDs in a single call to tag_get_data?

meltawila force-pushed the add_bcs branch from 31b5ba9 to bcbabc5 Compare February 12, 2026 03:08

meltawila added 2 commits March 23, 2026 20:21

rebased on top of upstream/devel

5e613ba

update tests

886d54e

meltawila force-pushed the add_bcs branch from bcbabc5 to 886d54e Compare March 24, 2026 01:22

update splitting logic and handle unspecified transmission surfaces c…

7b6eb7b

…orrectly

clang-format

eede5b7

meltawila marked this pull request as ready for review March 24, 2026 15:54

edit equivalent CSG model and added a test with transmission surface

585a259

aprilnovak requested changes Mar 26, 2026

View reviewed changes

gonuke suggested changes Mar 26, 2026

View reviewed changes


		const auto & boundary_info = getMooseMesh().getMesh().get_boundary_info();

		for (const auto * elem : getMooseMesh().getMesh().active_element_ptr_range())

Conversation

meltawila commented Feb 4, 2026

Uh oh!

moosebuild commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moosebuild commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage

Uh oh!

moosebuild commented Mar 24, 2026

Uh oh!

meltawila commented Mar 24, 2026

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gonuke left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moosebuild commented Feb 4, 2026 •

edited

Loading

moosebuild commented Feb 4, 2026 •

edited

Loading

gonuke left a comment •

edited

Loading