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

reworking Be assembly example #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

reworking Be assembly example #12

wants to merge 4 commits into from

Conversation

eepeterson
Copy link
Owner

This is just a streamlined version of the great work @Ebiwonjumi put in on #10 for only the fns_be part since that was the first one I saw in the PR. I saw that there were a number of copy/pasted model generating files, but for each benchmark I would like to have a single openmc python API input. I'm hoping the rework here of fns_be_build_xml.py, fns_be_build_xml_gh.py, and fns_be_build_xml_rxn_rate.py into a single fns_be.py file makes sense to people. There are a couple things I want to highlight about this approach below:

  • One model generating script per benchmark means materials, geometry, tallies, etc are all located in one spot (single source of truth). This will make it easier for reviewers to understand the benchmark and make it more likely to get included in things like CoNDERC faster.
  • This script has command line arguments for changing things one might want to change when running the benchmarks without modifying the source code like number of particles, batches, threads, and the tallies since it's difficult to converge all the tallies in the same simulation.
  • Let's leverage some of the best bits of the Python API like not specifically managing material or surface IDs as well as using intermediate variables for complicated region expressions.
  • It should follow PEP8 guidelines as best as is reasonable (admittedly I'm not perfect at this, but generally I follow it closely)

I haven't yet checked whether this gives us identical results as what @Ebiwonjumi has been showing so that would be something else we need to do and having a single output parsing script for each benchmark would help with this.

Once this is reviewed and merged, we can divvy up some of the other benchmarks and rework them into this format, but I think it is worth spending some time iterating on whether this is the right way we want to handle this before spending more time reworking inputs. I would appreciate input from everyone if they have the time so we can figure out a uniform way to proceed. @Ebiwonjumi @SteSeg @paulromano @pshriwise.

Copy link
Collaborator

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Looks pretty nice! Just a few line comments from me.

fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
fns_be/fns_be.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Ebiwonjumi
Copy link
Collaborator

The reasons why I initially had the inputs in three places for the flux, reaction rates and gamma heating are:

(1) didn't check yet if the trace nuclides I added in the _reaction_rate input would affect the flux significantly;
(2) for fair comparison with MCNP, since trace nuclides aren't added to material cards in MCNP transport.
(2) the flux spectra tally needed less neutron histories to have good statistics, compared to the histories needed to get <1% statistical error on the reaction rate and gamma heating.

It is best to have one input file for all tallies as you have done.

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.

3 participants