Repository navigation
Allow zero bins in tally triggers - #2928
Conversation
A test on the O16(n,p) reaction, which has a threshold just over 1e7 eV at 300 K.
...reproduced this using the NNDC data. Added more particles, and now the triggers are satisfied early as expected. |
paulromano
left a comment
There was a problem hiding this comment.
@tjlaboss Thanks for implementing this! The code looks good to me. Two general comments:
- I think a name like
ignore_zeroswould more clearly express the intent of the option compared toallow_zero. Are you OK with renaming it as such? - I see the point about skipping the first check, but that would eliminate the easy-to-remember notion that when using triggers,
settings.batchesis the minimum number of batches andsettings.trigger_max_batchesis the maximum. I personally think we should preserve the current behavior and users will just have to be cautious about the minimum number of batches chosen. The change to skip "interval 0" doesn't really eliminate the problem but rather defers it until "interval 1" where the same issue can still happen (all bins being zero).
|
This makes sense to me. I'll rename the parameter to |
|
Thanks @tjlaboss! Please ping me when the PR is ready for another look |
|
@paulromano, thanks for the review. The requested changes have been made and the CI tests have passed. |
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
With the ratio fixed, a zero-score bin skipped by ignore_zeros (openmc-dev#2928) enters the variance arm carrying the -1 sentinel from get_tally_uncertainty() and contributes sqrt(1 / threshold), so a variance trigger with ignore_zeros could never be satisfied for thresholds below 1. The old sqrt(ratio) code hid this. For std_dev and rel_err the sentinel gave a negative ratio that never won the maximum, so their behaviour is unchanged. Skip such bins explicitly instead of letting them into the metric switch. Parametrize the ignore_zeros test over the variance metric.
Resolve #2899
Description
Add the ability to ignore tally bins with zero scores when evaluating tally triggers.
allow_zerooption forTriggerclass and tallies.xml. (default: False)Checklist
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 29b0782,
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_zeroon 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
allow_zerois 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.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) ordistutils(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 toFalse.