Addition of a tally system and [Tallies] block

[nuclearkevin]

Reason

This PR implements a new tally system and an associated block in Cardinal's input syntax based on a discussion with @aprilnovak (ref. #922). This allows for the addition of multiple different tallies at once which have different scores, an example of which can be found below (tests/neutronics/tally_system/multi_diff.i):

[Problem]
  type = OpenMCCellAverageProblem
  verbose = true
  power = 1e4
  temperature_blocks = '100 200'
  density_blocks = '200'
  cell_level = 0
  initial_properties = xml

  # The global tally check is disabled because we have a loosely fitting unstructured mesh tally.
  normalize_by_global_tally = false

  source_rate_normalization = 'kappa_fission'

  [Tallies]
    [Cell]
      type = CellTally
      score = 'kappa_fission'
      blocks = '100 200'
    []
    [Mesh]
      type = MeshTally
      score = 'flux'
      mesh_translations = '0 0 0
                           0 0 4
                           0 0 8'
      mesh_template = ../meshes/sphere.e
    []
  []
[]

This is advantageous if different spatial filters are required for different scores. This PR will also (hopefully) make it easier to add new filters in the future (such as an EnergyFilter) without requiring the user to manually post-process tallies in the input file when they don't need certain scores binned according to a new filter.

Design

The new tally system adds the following new objects:

• TallyBase: A class which wraps a single OpenMC tally to provide common data types and virtual functions required by spatially dependant tallies. TallyBase classes are constructed by an OpenMCCellAverageProblem through the addTallyObject(...) function, which mirrors how MOOSE handles MooseObject construction in FEProblemBase. The virtual functions that must be overloaded by derived tallies (CellTally and MeshTally) are:
  • initializeTally(...): A function where derived tallies can implement functionality required to initialize an OpenMC tally. Called by the OpenMCCellAverageProblem in initializeTallies(...).
  • resetTally(...): A function where derived tallies can implement functionality required to reinit an OpenMC tally. Called by the OpenMCCellAverageProblem in resetTallies(...).
  • storeResults(...): A function where derived tallies can store their tally results in a field auxvariable. Called by the OpenMCCellAverageProblem in syncSolutions(...).
  • storeExternalResults(...): A function where derived tallies can store other requested tally results (unrelaxed / unrelaxed standard deviation) in a field auxvariable. Called by the OpenMCCellAverageProblem in syncSolutions(...).
• CellTally: A class derived from TallyBase which implements distributed cell tallies. Replaces the functionality of tally_type = cell in the OpenMCCellAverageProblem.
• MeshTally: A class derived from TallyBase which implements unstructured mesh tallies that may also be translated in space. Replaces the functionality of tally_type = mesh in the OpenMCCellAverageProblem.
• AddTallyAction: An Action which is responsible for adding TallyBase objects to an OpenMCCellAverageProblem. This action calls addTallyObject(...) when the simulation reaches the new add_tallies task during execution.
  • When the tally to be added is a CellTally, a single tally object is added.
  • When the tally to be added is a MeshTally, multiple tally objects may be added if the user specifies mesh translations. These multiple tallies all accumulate in the same set of auxvariables.

Multiple tallies of the same type are allowed by this system so long as they do not share scores. In general, multiple tally objects that share scores are not allowed as it becomes difficult to normalize the tallies due to possible bin overlap.

At present time this new system is NOT backwards compatible with previous versions of Cardinal. Attempting to use an input deck where a single tally is added in the [Problem] block will result in an error telling the user to use the [Tallies] block. If backwards compatibility is desirable, I can emulate the old system by having OpenMCCellAverageProblem add an AddTallyAction in it's constructor with the input parameters of the required tally.

Impact

• Allows for greater tally flexibility, both for users and for the addition of new features.
• Reduces the number of input parameters in OpenMCCellAverageProblem and decreases the overall complexity of the class.
• Breaks input decks which don't use the new tally system (at the moment).
  • This can be fixed if backwards compatibility is desired.

Other

This PR is unfortunately quite large due to the need to update the input syntax for every tutorial and test. None of the tests required re-golding, so I don't believe updating the syntax will cause any CI/CD related issues. The documentation website has also been updated to reflect the new system (including the tutorial explanations). I believe that I caught every reference of the old system, but if I missed something please let me know and I'll get it changed over.

[aprilnovak]

Awesome!! I'm also requesting a review from @pshriwise to get his feedback on this.

[moosebuild]

Job Documentation on 112309d wanted to post the following:

View the site here

This comment will be updated on new commits.

[aprilnovak]

How are the tallies normalized when we have > 1? I see there's a new parameter, global_tally_estimator. Would it instead make sense to normalize each tally individually? If you have a cell tally with a tracklength estimator, but it's normalized by a collision estimator, power won't be preserved as the user might expect, right?

[nuclearkevin]

The default at the moment is that tallies are normalized by a single global tally. If there is a mismatch between the global estimator and the estimators in the added tallies, the user is warned (lines 2727 to 2760 in OpenMCCellAverageProblem.C). In the case of a single tally, the global estimator is set to the estimator of the single local tally. The user still has the option to normalize by a local tally by setting normalize_by_global_tally to false.

My thought process at the time was to leave it up to the user on how they want their different tallies to get normalized. Thinking about it now, it would make more sense to set the default to normalizing by the local tallies due to this change.

[moosebuild]

Job Coverage on 112309d wanted to post the following:

Coverage

	b645e2	#931 112309
	Total	Total	+/-	New
Rate	93.52%	93.39%	-0.13%	94.32%
Hits	7269	7427	+158	598
Misses	504	526	+22	36

Diff coverage report

Full coverage report

This comment will be updated on new commits.

[aprilnovak]

Thanks @nuclearkevin! I think there's just one last item which occurred to me. It might be complicated.

We populate the _cell_to_elem map (and other objects in OpenMCCellAverageProblem) based on a std::set of the blocks given by tally_blocks (old syntax), temperature_blocks, and density_blocks. I'm wondering what happens if you have two CellTally objects, each defined on the same blocks, but with different scores. Are we re-adding duplicate elements to the _cell_to_elem map?

If so, I think you'd be able to detect this using the CellVolumeAux, to display the element volume each cell maps to. If things are wrong, then CellVolumeAux will give you different values depending on whether you had 1 tally with N scores, vs. N tallies with 1 score each (same block definitions for all of them).

Can you please test this out and see what happens?

[nuclearkevin]

@aprilnovak It looks like the cell volume reported by the CellVolumeAux doesn't change if a single CellTally with two scores is added vs two CellTally with a single score each (on shared blocks).

Postprocessor Values (single CellTally):

+----------------+----------------+----------------+----------------+
| time           | Cell_1_Vol     | Cell_2_Vol     | Cell_3_Vol     |
+----------------+----------------+----------------+----------------+
|   0.000000e+00 |   0.000000e+00 |   0.000000e+00 |   0.000000e+00 |
|   1.000000e+00 |   1.322128e+01 |   1.322128e+01 |   1.322128e+01 |
+----------------+----------------+----------------+----------------+

Postprocessor Values (two CellTally with different scores on shared blocks):

+----------------+----------------+----------------+----------------+
| time           | Cell_1_Vol     | Cell_2_Vol     | Cell_3_Vol     |
+----------------+----------------+----------------+----------------+
|   0.000000e+00 |   0.000000e+00 |   0.000000e+00 |   0.000000e+00 |
|   1.000000e+00 |   1.322128e+01 |   1.322128e+01 |   1.322128e+01 |
+----------------+----------------+----------------+----------------+

Also adding this to tests/neutronics/tally_system/multi_cell.i as an additional check just to be sure.

[aprilnovak]

Which volume_type did you use for CellVolumeAux? Just check that you used volume_type = mapped

[nuclearkevin]

Yep, I used volume_type = mappedfor the check.

[aprilnovak]

Great! This looks good-to-go. I'm just going to do a local checkout and run some of the tests, see if I can think of other error checks which may be missing.

@pshriwise how would you prefer to handle the timing of this PR in contrast to #923? #923 is for a GAIN project which has a close-out approaching soon, so I'll defer to Patrick on what order we should merge these in order to minimize any schedule delays with GAIN.

[aprilnovak]

Looks like each of the test gold files is rather large. Can we test this equivalently using just 1 pebble, instead of 3 with the CombinerGenerator? We can squash all the commits when it's merged, so don't worry about removing big files from the git history.

[nuclearkevin]

Updated the tests. The only major change is that moving to a single pebble requires check_tally_sum = false as the xml files still model three pebbles (so the global tally ends up being larger than the local tally).

[aprilnovak]

Was check_tally_sum=true working before? I'd be surprised because the meshes are not conformal to the pebbles, so even with all 3 included you'd still have slivers of pebbles outside the mesh but inside the CSG spheres which would contribute to the global tally.

[nuclearkevin]

Oh, I should've been more explicit. This is for tests that include CellTally objects since the Moose mirror only maps a single cell instead of all three. The mesh tally tests already had the check disabled.

[aprilnovak]

oh gotcha! How about we do this -- let's actually keep all 3 pebbles for the cell tests, since they would catch anything going wrong with instancing. But instead of an exodiff test, for those we can easily do a CSVDiff (PointValue for the tally value at the centroid of each pebble).

[nuclearkevin]

CellTally tests have been changed to CSVDiffs. I took the liberty of reverting tests/neutronics/tally_system/multi_mesh.i back to three pebbles to make sure we cover the case where there are multiple distributed mesh tallies.

[aprilnovak]

Sorry for the kind of piecemeal review process, but another thought came to mind. Can we consolidate some of the duplicate behavior in CellTally and MeshTally into the base class? For instance, the initializeTally method has a lot of duplicate code - I'd suggest we have virtual functions in the base class which derived classes can override. This should make code maintenance more straightforward while also streamlining an upcoming FET implementation for Cardinal that another student is working on.

[nuclearkevin]

No worries! It's a rather large PR that was submitted near the end of the summer, it was destined to take a while to review.

I could move the initializeTally(...) function into the base class and instead replace it with a virtual function called spatialFilter(...) (or something similar) which the derived tally objects can use to create the spatial filter that the class corresponds to. I can also consolidate the resetTally(...) function into the base class with a default implementation, but still allow derived tally objects to override it so they can add custom behavior (MeshTally needs to reset the mesh it uses, while CellTally does not).

I don't know if I can really move the functionality in storeResults(...) to the base class since it heavily depends on the derived tally object. I can definitely find a way to merge storeResults() and storeExternalResults() into a single function per derived tally to decrease the amount of code duplication though.

[aprilnovak]

That sounds like a good plan to me - I don't anticipate we'd be able to move absolutely everything, I'll trust your judgement on what makes sense to consolidate into the base class (a few obvious places stood out to me, which you've touched on).

[moosebuild]

Job Precheck on 72072c6 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/931/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 ecb42eeb5a28b94d4dddadaef9add2def01fad12