Description

Add the ability to ignore tally bins with zero scores when evaluating tally triggers.

New allow_zero option for Trigger class and tallies.xml. (default: False)
If there is a score without contributions, do not set the trigger threshold ratio to infinity if the trigger allows zero scores.
Ensure that a full trigger batch interval is run before evaluating triggers (see discussion).
Add a unit test. The O-16(n,p) reaction is tallied using an energy filter with two bins, one of which is entirely below the threshold energy. The low energy bin should always have a score of zero.
Update "Tallies Specification" documentation.

Checklist

[x] I have performed a self-review of my own code
[x] I have run clang-format (version 15) on any C++ source files (if applicable)
[x] I have followed the style guidelines for Python source files (if applicable)
[x] I have made corresponding changes to the documentation (if applicable)
[x] I have added tests that prove my fix is effective or that my feature works (if applicable)

Questions and Discussion

OpenMC's tally trigger behavior

After OpenMC completes inactive batches, it runs a few active batches to begin gathering tally statistics. Then, tally triggers are evaluated.

This can present a problem with the new 'allow_zero' option. When triggers are evaluated this early, tally bins may not yet have any scores. Normally, this sets the error metric to infinity, but when allow_zero == True, the trigger can "fire" prematurely if none of the tally bins have any hits yet. This can happen for rare events, such as shielding problems and small-xs reactions.

Currently, this check is performed immediately after those initial active batches are performed, regardless of the user's tally trigger 'trigger_batch_interval' setting; let's call this "interval 0". In commit 29b07827f8d48c4e8eb5385f58c777a1f8441787, check_triggers() is changed to skip "interval 0", so that the check is only performed after each full interval. This way, users can ensure a minimum number of active batches and avoid false positives when zero-score bins are allowed.

This does change the default tally trigger behavior. Since the default 'trigger_batch_interval' is 1, this will add 1 additional batch before checking tally triggers even if they do not enable allow_zero on any tallies. I think this is the best balance between straightforwardly addressing the problem and being unobtrusive to existing tally trigger users, but let's discuss.

Alternatives

One alternative would be to check if allow_zero is used on any tallies, and only skip the 0th interval if it is. The advantage is that this would then have no impact on existing workflows. The disadvantage is that the trigger checking behavior with and without the option would be inconsistent.
Another alternative would be to continue checking at the 0th interval, document that this can cause problems with rare events, and make it the user's responsibility.

Testing

Is the single unit test sufficient, or is a regression test necessary as well?

Bool

How do we want to handle Booleans in the XML, without using eval (unsafe) or distutils (deprecated)? Here, I just check if a lowercase string is in ("true", "1"), or ("false", "0"), and otherwise crash. This is similar to how bools are handled in settings.py, but with the inconsistency that an unrecognized string (e.g., "rtue" or "yes") won't default to False.

tjlaboss commented 1 month ago

Triggers unsatisfied, max unc./thresh. is 1.0000000000000002 for (n,p) in tally 3

~Wonder if this is floating point issue or a data library difference vs. the ENDF and JEFF libraries I tested on. Marking as draft...~

...reproduced this using the NNDC data. Added more particles, and now the triggers are satisfied early as expected.

tjlaboss commented 6 days ago

This makes sense to me. I'll rename the parameter to ignore_zeros, revert 29b07827f8d48c4e8eb5385f58c777a1f8441787, make a note in the documentation, and add more particles to the test case.

paulromano commented 6 days ago

Thanks @tjlaboss! Please ping me when the PR is ready for another look

tjlaboss commented 3 days ago

@paulromano, thanks for the review. The requested changes have been made and the CI tests have passed.

openmc-dev / openmc