Criticality search

[aprilnovak]

This PR adds an example of performing a criticality search using Cardinal, starting by the easiest form of criticality maneuvers:

• changing a material density
• changing the boron weight ppm in water.

Additional criticality search changes can be added as daughter classes of CriticalitySearchBase.

[nuclearkevin]

Looks good, my requested changes mostly involve implementation details. The design looks easy to extend to other search quantities (boron concentration, rod height, drum rotation, etc.)!

[moosebuild]

Job Documentation, step Sync to remote on ea3c7c9 wanted to post the following:

View the site here

This comment will be updated on new commits.

[aprilnovak]

This is ready for review - I added the borated water control as a second example.

[nuclearkevin]

Just a few minor comments / changes. The only major change I'd recommend would be to swap over to using https://github.com/OTU-Centre-for-SMRs/nuclear_data as a submodule instead of copy-pasting functions in (to avoid adding 3,000 lines of lookup tables). Otherwise, looks good to me!

[gonuke]

Thanks for the effort on this @aprilnovak - I definitely would not have produced a design quite this sophisticated.

I've taken a Quick Look at the code (i.e. not yet the docs or tests) and it all looks good to me.

I did find two small changes related to Nek that seemed out of place and I wanted to flag to see that it's what you intended.

[gonuke]

Just a few minor comments / changes. The only major change I'd recommend would be to swap over to using https://github.com/OTU-Centre-for-SMRs/nuclear_data as a submodule instead of copy-pasting functions in (to avoid adding 3,000 lines of lookup tables). Otherwise, looks good to me!

Yeah - I was definitely surprised to see that this didn't exist in MOOSE somewhere already.

[aprilnovak]

The only major change I'd recommend would be to swap over to using https://github.com/OTU-Centre-for-SMRs/nuclear_data as a submodule instead of copy-pasting functions in (to avoid adding 3,000 lines of lookup tables).

Yes, agreed! Thanks for moving that shared code into a submodule @nuclearkevin. I've updated this PR to add the new submodule.

[moosebuild]

Job Precheck, step Clang format on 2f85537 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/1184/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 4f4414a6dfb6d20fcf6bea4b70d951008384d1a5

[gonuke]

Thanks for clarifying the Nek changes. I've added a few comments final comments.

[gonuke]

An alternative to implementing other absent nuclides (Since they are extremely unlikely) is to tweak the documentation, as suggested in this comment

[gonuke]

This all looks great to me - thanks @aprilnovak - I'll get to work on implementing the control element transformation versions.

[nuclearkevin]

LGTM! Should be good to go once the CIVET recipe is updated to take into account the new submodule addition and the tests start running + passing.

[aprilnovak]

yep, waiting for the PR in civet_recipes to be merged.

[lewisgross1296]

Nothing major, just somethings I didn't fully understand and was hoping for clarification / addition of comments to the code.

Thanks for putting this together so rapidly @aprilnovak!

[pshriwise]

Passing thought: might be nice to allow a target k-eff to be specified (with a default of 1.0).

[gonuke]

Passing thought: might be nice to allow a target k-eff to be specified (with a default of 1.0).

This might be a straightforward addition to include here, esp if it's optional with a default value of 1.0.

[aprilnovak]

I'm going to leave adding the S(a,b) tables to a future PR since there's not a nice API to do it on the OpenMC side. Updated the docs to reflect this.

[moosebuild]

Job Test OpenMC on 01a3e0f : invalidated by @aprilnovak

[moosebuild]

Job Test OpenMC on 2a2f832 : invalidated by @aprilnovak

[moosebuild]

Job Test OpenMC on 3dac017 : invalidated by @aprilnovak

[nuclearkevin]

I believe something changed in how the critical boron search is performed between when you generated the gold file for the test and now (which is why the test is failing). It might need a regold.

[aprilnovak]

it's passing fine on my local machine - I think it's some indeterminisim as the number of batches increases (locally, I'm seeing the same values for k print out up until a point, so I tried decreasing the number of batches in my latest push). Will keep trying!

[gonuke]

Passing thought: might be nice to allow a target k-eff to be specified (with a default of 1.0).

We'd be happy to PR this little tweak to your branch.

We're getting close to having the rotational search ready. Should we PR that to this branch? or wait for this to be merged first?

[aprilnovak]

That would be great if you wanted to add, thank you! Sorry this has slipped a bit on my to-do list (still need to figure out why the test is passing for me locally but not on CIVET). Please feel free to go ahead and add a commit with the k \ne 1 option!

I'd suggest we merge this one first and then a follow-on one with the cell modification options.

[gonuke]

That would be great if you wanted to add, thank you! Sorry this has slipped a bit on my to-do list (still need to figure out why the test is passing for me locally but not on CIVET). Please feel free to go ahead and add a commit with the k \ne 1 option!

I'd suggest we merge this one first and then a follow-on one with the cell modification options.

I PR'd this into your branch for you to review and merge there.

[gonuke]

Note: OpenMC's tolerance for keff search is a relative tolerance. As far as we can tell, the MOOSE implementation of Brents Method interprets the tolerance as an absolute tolerance on the independent variable. When comparing "gold" solutions between Cardinal and OpenMC, this may matter, but may be moot when only relying on one or the other for solutions, as long as the user knows which.

[lewisgross1296]

Once OpenMC finishes up its search refactor (PR), we should be able to specify either relative or absolute tolerance in the scipy run args, which will making testing/golding against MOOSE easier.

[aprilnovak]

I have no idea what github is showing me, I'm not sure if it's a glitch or something went really wrong... anyways, I've re-opened the same branch over here: #1233

[moosebuild]

Job Test on 19afc12 : invalidated by @aprilnovak

[aprilnovak]

Once OpenMC finishes up its search refactor (openmc-dev/openmc#3569), we should be able to specify either relative or absolute tolerance in the scipy run args, which will making testing/golding against MOOSE easier.

The test failures here aren't anything to do with a relative error/tolerance -- the issue is that when I run OpenMC on my machine where I created the test gold file, the random number stream/etc. is different than when the test runs on Civet.

[lewisgross1296]

Is it possible to use openmc.Settings.seed in the test? Also are any of the tests using OpenMP parallelism? In my experience to get identical searches, you need to use only MPI parallelism

[aprilnovak]

I don't think the seed is relevant (the tests are using the same exact seed). Also, I believe OpenMC should be reproducible even if using OpenMP (but FWIW, the tests on CIVET use MPI only I believe, unless otherwise instructed, which we are not in these new tests being added).