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

adding SFR test cases #12

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

adding SFR test cases #12

wants to merge 22 commits into from

Conversation

magnoxemo
Copy link

No description provided.

Copy link

@lewisgross1296 lewisgross1296 left a comment

Choose a reason for hiding this comment

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

A few things that can be applied throughout. Try and look for repeated code that could be turned into functions.

Also, you should probably remove the .jitcache (not something you need or should even version).

Worth considering doing this PR as a few other ones (for the various assemblies).

Comment on lines 25 to 27
###################################################################
########################## Materials ########################
###################################################################

Choose a reason for hiding this comment

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

Let's consider making a function that returns a dictionary of materials create_SFR_materials() and converting use of the material objects to material_dict["mat_name"]

Comment on lines 45 to 61
pattern ='0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 0 ;
0 0 0 0 0 0 0 0 0 '

Choose a reason for hiding this comment

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

I personally like to use one space between the entries in the pattern. This looks a little wonky (though I'm sure it generates the correct mesh.

Comment on lines 84 to 85
inner_fuel_material= openmc.Material.mix_materials([u235, u238, pu238, pu239, pu240, pu241, pu242, am241, o16],[0.0019, 0.7509, 0.0046, 0.0612, 0.0383, 0.0106, 0.0134, 0.001, 0.1181],'wo')
cladding_material = openmc.Material.mix_materials([cu63,Al2O3], [0.997,0.003], 'wo')

Choose a reason for hiding this comment

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

Probably applicable other places, but we should maybe Black format your python code

Comment on lines 100 to 102
fuel_or = openmc.ZCylinder(surface_id=1, r=0.943/2)
clad_ir = openmc.ZCylinder(surface_id=2, r=0.973/2)
clad_or = openmc.ZCylinder(surface_id=3, r=1.073/2)

Choose a reason for hiding this comment

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

Not sure you need these surface_ids here. Also we should probably use variables to define the radii

Comment on lines 9 to 22
# Create the argument parser
ap = ArgumentParser(description="SFR Pincell Model Generator")
ap.add_argument('-n', dest='n_axial', type=int, default=100, help='Number of cells in the Z direction')
ap.add_argument('-e', '--shannon_entropy', action='store_true', help='Add Shannon entropy mesh')
ap.add_argument('--height', dest='height_of_the_core', type=float, default=30.0, help='Height of the reactor core')

# Parse the arguments
arguments = ap.parse_args()

arguments=ap.parse_args()

N=arguments.n_axial
height=arguments.height_of_the_core
shannon_entropy=arguments.shannon_entropy

Choose a reason for hiding this comment

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

let's do this in a main() function and throw a

if __name__ == "__main__":
    main()

at the bottom.

I think a function like build_SFR_model() that actually does all the model generation should be called form main()

Comment on lines 188 to 207
# settings
settings=openmc.Settings()
settings.batches=200
settings.inactive=40
settings.particles=20000


if (shannon_entropy):
entropy_mesh = openmc.RegularMesh()
entropy_mesh.lower_left = lower_left
entropy_mesh.upper_right = upper_right
entropy_mesh.dimension = (10, 10, 20)
settings.entropy_mesh = entropy_mesh

settings.temperature = {'default': 280.0 + 273.15,
'method': 'interpolation',
'range': (294.0, 3000.0),
'tolerance': 1000.0}

settings.export_to_xml()

Choose a reason for hiding this comment

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

we can make a settings function too

Comment on lines 1 to 11
#----------------------------------------------------------------------------------------
# Assembly geometrical information
#----------------------------------------------------------------------------------------
pitch = 1.25984
height = 300.0
r_fuel = 0.4715
t_gap = 0.0150
r_clad_inner = 0.4865
t_clad = 0.05
edge_length =12.1705
#----------------------------------------------------------------------------------------

Choose a reason for hiding this comment

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

would be nice to have a common_input.i and

!include common_input.i

at the top

Comment on lines 150 to 155
# Define a lattice for outer assemblies
out_lat = openmc.HexLattice(lattice_id=1, name='outer assembly')
out_lat.center = (0., 0.)
out_lat.pitch = (21.08/17,)
out_lat.orientation = 'y'
out_lat.outer = sodium_mod_u

Choose a reason for hiding this comment

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

consider using a 3D lattice, you can look at the GCMR repo to see an example of how to do this and still have density feedback


# Create the argument parser
ap = ArgumentParser(description="SFR Pincell Model Generator")
ap.add_argument('-n', dest='n_axial', type=int, default=100, help='Number of cells in the Z direction')
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a default of 1? I think that makes sense as a default because that would be the best performance choice if not needing extra cells for depletion/feedback.


#Material mixtures

inner_fuel_material= openmc.Material.mix_materials([u235, u238, pu238, pu239, pu240, pu241, pu242, am241, o16],[0.0019, 0.7509, 0.0046, 0.0612, 0.0383, 0.0106, 0.0134, 0.001, 0.1181],'wo')
Copy link
Contributor

@aprilnovak aprilnovak Feb 26, 2025

Choose a reason for hiding this comment

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

This would be much more efficient (less code) to create this object using a single openmc.Material (as long as you know the overall fuel density from somewhere?)


for i in range (N):

layer=+z_plane[i]&-z_plane[i+1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This may have been from an example, but why not use a lattice in the z-direction? I would expect the tracking performance to be a little bit better that way, as opposed to N unique cells.

Choose a reason for hiding this comment

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

Agreed. Check out my GCMR code for an example with a 3D lattice. There might be some depletion things you don't need in there but it should serve as a good base. IIRC there's some cardinal tutorials that do it too

Copy link

Choose a reason for hiding this comment

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

This may be based on the approach that @nuclearkevin used for LWR problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I actually didn't know that lattices could be defined in 3D. I'll open a PR at some point over the next few days to update the LWR cases so they use a 3D lattice.

direction = '0.0 0.0 1.0'

bottom_boundary = '10001'
top_boundary = '10000'
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the reactor module likes to use the sideset 10000 as the default outer boundary (if my memory is correct). Did you check that there's not any other existing sidesets numbered 10000 that might be conflicting with this one?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you April. I removed that default top and bottom name and generated the mesh again which I think looks OK. But I hope someone else could cross check maybe?

[]
[]

[Adaptivity]
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we may want to run a whole series of models through different AMR strategies, can you please update these .i files to use the !include strategy idea from #1?

Copy link
Author

Choose a reason for hiding this comment

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

All the amr studies I have for the SFR models are direct implementation of Kevin's value_jump_rel_error. I can remove the adaptivity block from my SFR models since value_jump_rel_error already does it?

Choose a reason for hiding this comment

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

I think April is referring to this

cardinal_amr/README.md

Lines 20 to 25 in 221a7f1

In addition to these models, this repository also contains a series of AMR algorithms - these can be found in `/amr_strategies/*`. To apply one of these AMR strategies to a test model in this repository, follow the steps below:
1. Add an `!include STRATEGY_NAME` to the beginning of the `openmc.i` file in one of the model sub directories, where `STRATEGY_NAME` is the name of a file in `amr_strategies`.
2. Run `python make_openmc_model.py`.
3. Run `cardinal-opt -i mesh.i --mesh-only`.
4. Run `mpiexec -n NUM_PROC cardinal-opt openmc.i --n-threads=NUM_THREADS`, replacing `NUM_PROC` with the number of MPI processors and `NUM_THREADS` with the number of OpenMP threads.

@gonuke
Copy link

gonuke commented Feb 26, 2025

In general, I encourage you to look at the changes that @nuclearkevin made during review to the LWR cases and see how you would make changes to match the design patterns used there.

@gonuke
Copy link

gonuke commented Feb 26, 2025

The *.so files shouldn't be here. You might want to update the .gitignore and get used to adding files explicitly to each commit rather than git add -A (or other shortcuts that can risk adding too many files)

@magnoxemo
Copy link
Author

I think this is ready for second round of review .

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.

5 participants