Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MG particle speed getter and delayed fission neutron emission #2898

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

ilhamv
Copy link

@ilhamv ilhamv commented Mar 8, 2024

Description

Add MG particle speed getter and delayed fission neutron emission. This extends OpenMC's capability to run autonomous problems accurately, such as neutron pulse experiments.

Input for settings.max_secondaries is added with the default (originally hard-coded) value of 10000. This allows users to set up buffers to run near-critical or super-critical time-dependent problems (accompanied by time_cutoff, of course).

Fixes #2885

cc: @gridley

Checklist

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

Verification

MG test

Here is the result of a pulsed infinite homogeneous 361-group problem, where the MGXS is generated from the pincell example:

shem361

The "analytical" reference solution is also shown for verification. The simulation was run in 10 batches with 100k particles per batch.

CE test

Here is the result of the jezebel example pulsed with 2 MeV isotropic point source at the origin:

jez_n

The simulation was run in 10 batches with 100k particles per batch.

Estimates of neutron inverse period, or time constant $\alpha$, can be naively calculated from two consecutive data points:

jez_alpha

Experimental value for the prompt neutron time constant ($\alpha_p$ = -640 +- 10 /ms) is also shown above for verification.

Note: In running this Jezebel problem, I had to increase the secondary bank size limit from the default value 10k to 100k.

@ilhamv ilhamv requested a review from nelsonag as a code owner March 8, 2024 16:16
@ilhamv ilhamv marked this pull request as draft March 8, 2024 16:16
@ilhamv
Copy link
Author

ilhamv commented Mar 8, 2024

The additional step of sampling delayed emission time shifts the random number sequence used in the rest of the random walk, which would make some (if not most) of the regression tests fail.

Is revising the regression test keys an option?

If not, the solution would be to only perform the delayed time sampling if a time-dependent problem is run (i.e., if TimeFilter is used in at least one tally).

@gridley
Copy link
Contributor

gridley commented Mar 10, 2024

Thanks @ilhamv! I believe that we should be consuming that extra random number to sample the delayed group emission time, so I'm going to tie @paulromano into this to put it on his radar. I think typically, in the past, people have let a few proposed changes accrue before changing the RNG stream. At that point some statistical equivalence testing is done between the two versions, and all the tests get updated.

If we're tracking particle times during normal simulation, the delayed emission times should not be incorrect, IMO. Correctness should not be an option that's off by default--this was the same principle that guided the decision of having resonance upscattering turned on by default, in contrast to most other CE MC codes.

Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only requesting a change on a typo which was already there but is in a line that you modified; other than that it all looks good to me. Going to wait on @paulromano for his thoughts on updating the tests.

docs/source/methods/neutron_physics.rst Outdated Show resolved Hide resolved
src/particle.cpp Show resolved Hide resolved
src/physics.cpp Show resolved Hide resolved
@gridley
Copy link
Contributor

gridley commented Mar 10, 2024

By the way, I think the code here to simulate the transient pulse would be worth including in the examples section. Would you want to include it as part of this PR?

Co-authored-by: Gavin Ridley <gavin.keith.ridley@gmail.com>
@ilhamv
Copy link
Author

ilhamv commented Mar 11, 2024

If we're tracking particle times during normal simulation, the delayed emission times should not be incorrect, IMO. Correctness should not be an option that's off by default--this was the same principle that guided the decision of having resonance upscattering turned on by default, in contrast to most other CE MC codes.

I really like that principle. 👍🏽

By the way, I think the code here to simulate the transient pulse would be worth including in the examples section. Would you want to include it as part of this PR?

Sure, I'll add the example as part of this PR.
Thanks, @gridley!

@ilhamv ilhamv marked this pull request as ready for review March 13, 2024 20:22
@ilhamv
Copy link
Author

ilhamv commented Apr 3, 2024

There was a little bug in the use of MG_mode + collision estimator + survival_biasing: 6ca1ae0

@gridley @nelsonag

(Note: I had to use the collision estimator due to the current limitation of MeshFilter + TimeFilter).

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilhamv Thanks for this PR and please accept my apologies that I haven't gotten around to properly reviewing it until now. I just made a few cosmetic updates on your branch and also took care of a merge conflict.

As @gridley pointed out, the additional random number that is sampled here for the delayed neutron time will cause the overall stream of random numbers to be different, which will fail most if not all of our regression tests. However, the guiding principle that @gridley put to words is indeed one of our tenets for OpenMC:

Correctness should not be an option that's off by default

So, if we really need to update our regression test results in the name of correctness, so be it. However, first I'll ask whether there are other changes you have in mind that would also result in more random number stream changes, in which case we might want to group them together as we've done previously.

@gridley
Copy link
Contributor

gridley commented Apr 24, 2024

Paul, that's great to hear, because I have had this other change on my radar to change the random number stream. We assume target nuclei are stationary above a certain energy threshold, and there's this recent paper that shows it introduces some non-negligible error for systems with finely distributed resonant material like TRISO or cracked MSR moderator.

The asymptotic form of the target velocity should just be to sample directly from Maxwell-Boltzmann, i.e. x y and z velocities are normally distributed. We should fall back to that rather than stationarity. The authors said it's like a 6% slowdown by always sampling the target motion, but so using some asymptotic form might be helpful in that case.

https://arxiv.org/abs/2403.14486

@paulromano
Copy link
Contributor

Ah yes, thanks for bringing that up @gridley. I did talk to the authors of that paper so I'm aware of the issue at hand.

@ilhamv
Copy link
Author

ilhamv commented Apr 27, 2024

No problem, @paulromano. Thanks!

I think that is the only change needed in the random number stream for analog time-dependent simulations. Unless there are other reactions that produce delayed particles (neutron/photon)..?

Some other items that I want to bring up for consideration (not directly related to this PR, though):

  • To get the track-length estimator working properly for tallying with both time and mesh filters, would it make sense to make the mesh filter 4D (x,y,z,t)?
  • Currently, the weight cutoff can not be set to zero. However, I see that in certain time-dependent problems where the neutron population decreases significantly in time, we do want to set the weight cutoff to zero (no cutoff) and instead set the time cutoff to a time range of interest. Would it be a good idea to allow users to do that? If it is a good idea, I can push a commit where I just removed the last argument in cv.check_greater_than('weight cutoff', cutoff[key], 0.0, True).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing MG particle speed getter and delayed fission neutron emission
3 participants