Skip to content

Conversation

@ZoeRichter
Copy link
Collaborator

Summary of changes

This is an improved version of the infinite lattice model PR. I have both removed any large files from the commit history and simplified the BCC unit cell models, cutting down on the number of files in general. This time I also included the bash script I've been using to run everything/move files/etc.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ZoeRichter ZoeRichter self-assigned this Aug 18, 2025
@ZoeRichter ZoeRichter added Comp:Core This issue has to do with the main bulk of the code or document. (methods, main content) Comp:Input This issue has to do with the input component of the code or document. (input parameters, prep) Comp:Output This issue has to do with the output component of the code or document. (writing to databases, etc.) labels Aug 18, 2025
@ZoeRichter ZoeRichter added this to the Prelim milestone Aug 18, 2025
Copy link
Member

@katyhuff katyhuff left a comment

Choose a reason for hiding this comment

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

Thanks @ZoeRichter , this is getting better.
Your code is very "WET" which is the antithesis of the DRY principle. Don't repeat yourself. I think you could reduce the code in this PR by a factor of 10 just by indexing things instead of using numbered strings everywhere.

dep_t = res.get_times()
step_comps = [res.export_to_materials(i,
path=mat_file)[0].get_nuclide_densities()
for i in range(len(dep_t))]
Copy link
Member

Choose a reason for hiding this comment

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

Is this indentation right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes - the second line is still part of the 'export_to_materials' term

path=mat_file)[0].get_nuclide_densities()
for i in range(len(dep_t))]

comp1249days = {}
Copy link
Member

Choose a reason for hiding this comment

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

Is the name of this variable the only thing that's different between these various bcc_model.py files?
If so, maybe the number of days can be a input variable for this script and we could instead have many fewer bcc_model.py files in the repo ... Maybe that's not helpful, but it's always worth thinking about how to avoid replicated code (because replicated code replicates bugs -- see Robert C. Martin, Clean Code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the depletion step the compositions are based on are different each time, which is why I redefine that single material each time. Materials that all the bcc models share are made by the gen_inputs script one level up, then pulled in here. That way, the full list of materials can be used by the openmc model when the geometry information is read in.

dep_t = res.get_times()
step_comps = [res.export_to_materials(i,
path=mat_file)[0].get_nuclide_densities()
for i in range(len(dep_t))]
Copy link
Member

Choose a reason for hiding this comment

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

Is this indentation right?
The code in this gen_inputs.py script seems pretty similar to the code in the bcc_inputs.py. Maybe some of these tasks should be callable modules that you can import instead of copy/pasting the same code into multiple files?

Comment on lines 17 to 75
pass01 = {}
tot_time01 = sum(dep_t[0:19])
for i, step in enumerate(step_comps[0:19]):
for k, v in step.items():
if k in pass01:
pass01[k] += v[1]*(dep_t[i]/tot_time01)
else:
pass01[k] = {}
pass01[k] = v[1]*(dep_t[i]/tot_time01)

pass12 = {}
tot_time12 = sum(dep_t[19:26])
for i, step in enumerate(step_comps[0:19]):
for k, v in step.items():
if k in pass12:
pass12[k] += v[1]*(dep_t[i+19]/tot_time12)
else:
pass12[k] = {}
pass12[k] = v[1]*(dep_t[i+19]/tot_time12)

pass23 = {}
tot_time23 = sum(dep_t[26:31])
for i, step in enumerate(step_comps[26:31]):
for k, v in step.items():
if k in pass23:
pass23[k] += v[1]*(dep_t[i+26]/tot_time23)
else:
pass23[k] = {}
pass23[k] = v[1]*(dep_t[i+26]/tot_time23)

pass34 = {}
tot_time34 = sum(dep_t[31:36])
for i, step in enumerate(step_comps[31:36]):
for k, v in step.items():
if k in pass34:
pass34[k] += v[1]*(dep_t[i+31]/tot_time34)
else:
pass34[k] = {}
pass34[k] = v[1]*(dep_t[i+31]/tot_time34)

pass45 = {}
tot_time45 = sum(dep_t[36:41])
for i, step in enumerate(step_comps[36:41]):
for k, v in step.items():
if k in pass45:
pass45[k] += v[1]*(dep_t[i+36]/tot_time45)
else:
pass45[k] = {}
pass45[k] = v[1]*(dep_t[i+36]/tot_time45)

pass56 = {}
tot_time56 = sum(dep_t[41:])
for i, step in enumerate(step_comps[41:]):
for k, v in step.items():
if k in pass56:
pass56[k] += v[1]*(dep_t[i+41]/tot_time56)
else:
pass56[k] = {}
pass56[k] = v[1]*(dep_t[i+41]/tot_time56)
Copy link
Member

Choose a reason for hiding this comment

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

Variable names with numbers in them are usually an indication that a data structure that is indexed by those numbers would be superior. This repeated code could be reduced if you create dictionary of dictionaries (passes) that's indexed by the pass number (that is, the keys would be 1,12,23, etc. and the values would be what you are currently calling pass01, pass12, etc.). Then, you could loop through the major key/value pairs before looping through the minor ones. That would allow you to collapse this whole set of multiple loops into one. Consider turning these minor loops into a function of their own as well and it will simplify things.

Copy link
Member

Choose a reason for hiding this comment

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

The same applies to tot_time01 etc.

Comment on lines 77 to 99
uco01= openmc.Material(name='UCO_01')
uco01.set_density('g/cm3', 10.4)
uco01.add_components(pass01, percent_type = 'ao')

uco12= openmc.Material(name='UCO_12')
uco12.set_density('g/cm3', 10.4)
uco12.add_components(pass12, percent_type = 'ao')

uco23= openmc.Material(name='UCO_23')
uco23.set_density('g/cm3', 10.4)
uco23.add_components(pass23, percent_type = 'ao')

uco34= openmc.Material(name='UCO_34')
uco34.set_density('g/cm3', 10.4)
uco34.add_components(pass34, percent_type = 'ao')

uco45= openmc.Material(name='UCO_45')
uco45.set_density('g/cm3', 10.4)
uco45.add_components(pass45, percent_type = 'ao')

uco56= openmc.Material(name='UCO_56')
uco56.set_density('g/cm3', 10.4)
uco56.add_components(pass56, percent_type = 'ao')
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. Here is what I mean (though there may be some syntax errors below, this is the idea.)

ucos = {}
pass_idxs = [1, 12, 23, 34, 45, 56] 
for idx in pass_idxs:
  mat_name = 'UCO_' + str(idx).zfill(2)  
  ucos[idx]= openmc.Material(name=mat_name)
  ucos[idx].set_density('g/cm3', 10.4)
  ucos[idx].add_components(passes[idx], percent_type = 'ao')

Comment on lines 204 to 322
'''
# iteration 0 (other sims branch from here)
plot_isos("Depleting Pebble Isotopics vs Time: Initial BCC Model (All Fresh), i0", 'Concentration',
'Time', 'iteration-0',
'inf_lat_dep/iter0/depletion_results.h5', '1')

# distinct corners
#i1 through i4:
plot_isos("Depleting Pebble Isotopics: Passwise-BCC Model, i1",
'Concentration',
'Time', 'passwise-i1',
'inf_lat_dep/dep-center/passwise/iter1/depletion_results.h5',
'13')

plot_isos("Depleting Pebble Isotopics: Passwise-BCC Model, i2",
'Concentration',
'Time', 'passwise-i2',
'inf_lat_dep/dep-center/passwise/iter2/depletion_results.h5',
'13')

plot_isos("Depleting Pebble Isotopics: Passwise-BCC Model, i3",
'Concentration',
'Time', 'passwise-i3',
'inf_lat_dep/dep-center/passwise/iter3/depletion_results.h5',
'13')

plot_isos("Depleting Pebble Isotopics: Passwise-BCC Model, i4",
'Concentration',
'Time', 'passwise-i4',
'inf_lat_dep/dep-center/passwise/iter4/depletion_results.h5',
'13')

# avg corners
#i1 through i4:
plot_isos("Depleting Pebble Isotopics: Corewise-BCC Model, i1",
'Concentration',
'Time', 'corewise-i1',
'inf_lat_dep/dep-center/corewise/iter1/depletion_results.h5',
'14')

plot_isos("Depleting Pebble Isotopics: Corewise-BCC Model, i2",
'Concentration',
'Time', 'corewise-i2',
'inf_lat_dep/dep-center/corewise/iter2/depletion_results.h5',
'14')

plot_isos("Depleting Pebble Isotopics: Corewise-BCC Model, i3",
'Concentration',
'Time', 'corewise-i3',
'inf_lat_dep/dep-center/corewise/iter3/depletion_results.h5',
'14')

plot_isos("Depleting Pebble Isotopics: Corewise-BCC Model, i4",
'Concentration',
'Time', 'corewise-i4',
'inf_lat_dep/dep-center/corewise/iter4/depletion_results.h5',
'14')

#comparisons:

# passwise vs avg: i1
compare_isos("Passwise vs Corewise BCC Depleting Pebble Isotopics: i1",'Time',
'i1-passwise-vs-corewise',
'inf_lat_dep/dep-center/passwise/iter1/depletion_results.h5',
'13', 'Passwise',
'inf_lat_dep/dep-center/corewise/iter1/depletion_results.h5',
'14', 'Corewise')

#passwise vs avg: i2
compare_isos("Passwise vs Corewise BCC Depleting Pebble Isotopics: i2",'Time',
'i2-passwise-vs-corewise',
'inf_lat_dep/dep-center/passwise/iter2/depletion_results.h5',
'13', 'Passwise',
'inf_lat_dep/dep-center/corewise/iter2/depletion_results.h5',
'14', 'Corewise')
#i3
compare_isos("Passwise vs Corewise BCC Depleting Pebble Isotopics: i3",'Time',
'i3-passwise-vs-corewise',
'inf_lat_dep/dep-center/passwise/iter3/depletion_results.h5',
'13', 'Passwise',
'inf_lat_dep/dep-center/corewise/iter3/depletion_results.h5',
'14', 'Corewise')
#i4

compare_isos("Passwise vs Corewise BCC Depleting Pebble Isotopics: i4",'Time',
'i4-passwise-vs-corewise',
'inf_lat_dep/dep-center/passwise/iter4/depletion_results.h5',
'13', 'Passwise',
'inf_lat_dep/dep-center/corewise/iter4/depletion_results.h5',
'14', 'Corewise')

#check convergence

#passwise:

fnames1 = ['inf_lat_dep/iter0/depletion_results.h5',
'inf_lat_dep/dep-center/passwise/iter1/depletion_results.h5',
'inf_lat_dep/dep-center/passwise/iter2/depletion_results.h5',
'inf_lat_dep/dep-center/passwise/iter3/depletion_results.h5',
'inf_lat_dep/dep-center/passwise/iter4/depletion_results.h5']
mat_ids1 = ['1', '13', '13', '13', '13']
stepcolors= ['turquoise', 'teal', 'orchid', 'purple', 'firebrick']

check_converge("Convergence Check using Depleting Pebble: Passwise BCC Model",
'Concentration', 'Time', 'passwise-converge', fnames1, mat_ids1,
stepcolors)

fnames2 = ['inf_lat_dep/iter0/depletion_results.h5',
'inf_lat_dep/dep-center/corewise/iter1/depletion_results.h5',
'inf_lat_dep/dep-center/corewise/iter2/depletion_results.h5',
'inf_lat_dep/dep-center/corewise/iter3/depletion_results.h5',
'inf_lat_dep/dep-center/corewise/iter4/depletion_results.h5']
mat_ids2 = ['1', '14', '14', '14', '14']

check_converge("Convergence Check Using Depleting Pebble: Corewise BCC Corners",
'Concentration', 'Time', 'corewise-converge', fnames2, mat_ids2,
stepcolors)

'''
Copy link
Member

Choose a reason for hiding this comment

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

Is this all commented out because it's wrong? If it's not ready for prime time, let's not merge it, eh?

Comment on lines 324 to 337
corewise_sp_fnames = ['bcc_phys/corewise/0days/statepoint.h5',
'bcc_phys/corewise/249days/statepoint.h5',
'bcc_phys/corewise/499days/statepoint.h5',
'bcc_phys/corewise/749days/statepoint.h5',
'bcc_phys/corewise/999days/statepoint.h5',
'bcc_phys/corewise/1249days/statepoint.h5',
'bcc_phys/corewise/1549days/statepoint.h5']
passwise_sp_fnames = ['bcc_phys/passwise/0days/statepoint.h5',
'bcc_phys/passwise/249days/statepoint.h5',
'bcc_phys/passwise/499days/statepoint.h5',
'bcc_phys/passwise/749days/statepoint.h5',
'bcc_phys/passwise/999days/statepoint.h5',
'bcc_phys/passwise/1249days/statepoint.h5',
'bcc_phys/passwise/1549days/statepoint.h5']
Copy link
Member

Choose a reason for hiding this comment

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

This cries out for loops.

Comment on lines +339 to +341
out_fnames = ['0days', '249days', '499days',
'749days', '999days', '1249days', '1549days']

Copy link
Member

Choose a reason for hiding this comment

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

loop, DRY.

Comment on lines +12 to +102
cd ../passwise
python3 gen_inputs.py

cd iter1
python3 bcc_model.py
cp depletion_results.h5 ../iter2/i1-dep-res.h5

cd ../iter2
python3 bcc_model.py
cp depletion_results.h5 ../iter3/i2-dep-res.h5

cd ../iter3
python3 bcc_model.py
cp depletion_results.h5 ../iter4/i3-dep-res.h5

cd ../iter4
python3 bcc_model.py
cp depletion_results.h5 ~/ghastly/openmc_models/bcc_phys/passwise/i4-dep-res.h5

cd ~/ghastly/openmc_models/inf_lat_dep

cd ../corewise
python3 gen_inputs.py

cd iter1/
python3 bcc_model.py
cp depletion_results.h5 ../iter2/i1-dep-res.h5

cd ../iter2
python3 bcc_model.py
cp depletion_results.h5 ../iter3/i2-dep-res.h5

cd ../iter3
python3 bcc_model.py
cp depletion_results.h5 ../iter4/i3-dep-res.h5

cd ../iter4
python3 bcc_model.py
cp depletion_results.h5 ~/ghastly/openmc_models/bcc_phys/corewise/i4-dep-res.h5

cd ~/ghastly/openmc_models/bcc_phys

cd passwise
python3 gen_inputs.py

cd /0days/
python3 bcc_model.py

cd ../249days/
python3 bcc_model.py

cd ../499days/
python3 bcc_model.py

cd ../749days/
python3 bcc_model.py

cd ../999days/
python3 bcc_model.py

cd ../1249days/
python3 bcc_model.py

cd ../1549days/
python3 bcc_model.py

cd ~/ghastly/openmc_models/bcc_phys

cd corewise
python3 gen_inputs.py

cd /0days/
python3 bcc_model.py

cd ../249days/
python3 bcc_model.py

cd ../499days/
python3 bcc_model.py

cd ../749days/
python3 bcc_model.py

cd ../999days/
python3 bcc_model.py

cd ../1249days/
python3 bcc_model.py

cd ../1549days/
python3 bcc_model.py
Copy link
Member

Choose a reason for hiding this comment

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

there are better ways.

DRY.
create a list of strings without explicitly typing each of them and create a loop that runs through them. There are great shell commands like find and such that can be helpful for this. See the shell chapter in the book or check out the software carpentry workshop on the topic. You should be able to do most of this in two or 3 lines.

Comment on lines +36 to +44
for step in step_comps[1:4]:
for k, v in step.items():
if k in temp_comp:
temp_comp[k]['iso'] += v[1]
temp_comp[k]['count'] += 1
else:
temp_comp[k] = {}
temp_comp[k]['iso'] = v[1]
temp_comp[k]['count'] = 1
Copy link
Member

Choose a reason for hiding this comment

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

If you do nothing else, please make this if else statement a function that takes the range_start and range_end as variables so you don't copy paste this whole thing a dozen times in one file.

@ZoeRichter
Copy link
Collaborator Author

Added loops to condense parts of the input generating scripts, mostly for materials.

@ZoeRichter ZoeRichter requested a review from katyhuff October 2, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Comp:Core This issue has to do with the main bulk of the code or document. (methods, main content) Comp:Input This issue has to do with the input component of the code or document. (input parameters, prep) Comp:Output This issue has to do with the output component of the code or document. (writing to databases, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants