Extract the combined k-effective estimator and fix its two-estimate branch - #4113
Open
GuySten wants to merge 5 commits into
Open
Extract the combined k-effective estimator and fix its two-estimate branch#4113GuySten wants to merge 5 commits into
GuySten wants to merge 5 commits into
Conversation
GuySten
marked this pull request as ready for review
September 8, 2026 02:02
nuclearkevin
requested changes
Sep 8, 2026
nuclearkevin
left a comment
Member
There was a problem hiding this comment.
Thanks for this fix @GuySten! I'd like to test it in the delta tracking branch before approving - I'll try to get to that soon (currently rather busy with M&C papers).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
openmc_get_keff()computes the minimum variance estimate of k-effective fromthe collision, absorption and tracklength estimators. The calculation is
inlined in a function that also reads global tallies, builds the covariance and
handles the low-realization case, which makes it hard to test on its own.
This PR moves it into
combine_estimates()insrc/math_functions.cpp: a purefunction combining three correlated estimates of any quantity, with no
reference to k-effective or to the individual estimators.
openmc_get_keff()supplies the k-specific meaning.
The covariance is taken as a
StaticTensor2D<double, 3, 3>rather than adynamically shaped
Tensor, so its shape is checked at compile time and thecall cannot be made with a mismatched matrix. It is also stack allocated, which
matters for callers that combine more often than once per run.
The
n <= 3condition moves in too, since it is a property of the expressions(an n-3 term in one denominator, n-2 in the other). The function reports
whether a combination was formed; the caller supplies its own estimate when it
was not. What stays in
openmc_get_keff()is that random rayhas only one estimate, that the fallback is the average over generations, and
that a single realization reports an infinite standard deviation.
The fix
The two-estimate branch, used when two estimators coincide, has an error in its
standard deviation. Urbatsch's Eq. 40 in LA-12658-MS is written in terms of the
matrix S, while the code substitutes the sample covariance
Sigma = S / (n - 1). The mean is unaffected; the standard deviation is
understated by up to sqrt(n - 1).
Found and diagnosed by @nuclearkevin in #4016, which was closed as "next to
impossible to test in continuous-energy transport without delta tracking" and
folded into #3971. Extracting the calculation removes that obstacle, which is
why the two changes are together here.
The branch is reached only in multi-group mode with survival biasing, where the
collision and absorption estimators are identical, so
tests/regression_tests/mg_survival_biasingis regolded. No other resultschange.
Validation
Three correlated estimators of a known value (pi) are simulated. Each trial
draws n realizations, forms the sample means and covariance as
accumulate_tallies()andopenmc_get_keff()do, and combines them. Over40,000 trials the mean reported standard deviation is compared with the actual
spread of the combined estimate:
develop's two-estimate standard deviation is too small by a factor growing with
n -- 23x at n=1000 -- matching the two to three orders of magnitude reported in
#4016. The corrected expression tracks the truth to within a percent. Both
branches are unbiased in the mean.
Both branches read a few percent low at n=10. That is inherent to the
derivation: the variance is a finite-sample approximation for weights estimated
from the same realizations, leaving a residual of order 1/n (-4.5%, -1.5%,
-0.7%, -0.4% at n = 10, 20, 50, 100). About half the n=10 gap is also a
measurement artifact -- the estimate is mildly heavy-tailed there, and against
an interquartile spread the ratio is 0.978 rather than 0.954. Nothing is worth
doing about it: a standard deviation from n realizations carries an inherent
uncertainty of 1/sqrt(2(n-1)), 23.6% at n=10, so the systematic error sits far
inside the noise.
Validation script
Testing
tests/cpp_unit_tests/test_combine_estimates.cpptests the combinationdirectly rather than through a transport calculation:
three shifts the result identically -- both follow from the weights summing
to one.
independently in terms of S. This pins down the standard deviation:
agrees to 2e-15 after the fix, off by 90% before it.
2.008 between n = 100 and n = 400 after the fix, 3.85 before.
The middle two fail on develop.
Checklist
I have followed the style guidelines for Python source files (if applicable)I have made corresponding changes to the documentation (if applicable)