-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement file and database IO functions in OpenMCDepcode
#189
Conversation
- Add helper functions - Add tests for read_depleted_materials() and helper functions - add `saltproc_runtime_ref` directory in `openmc_data` - remove unneeded import statements - make importable time variables in app.py - add assertion for `depcode_input[ouput_path]` in test_app.py
d017f56
to
0c73c42
Compare
- add relevant tests - move SerpentDepcode.read_plaintext_file to Depcode.read_plaintext_file - fix call to `_check_for_material_names` in OpenMCDepcode - fix `write_runtime_input` in OpenMCDepcode - simplify `file_interface_openmc` integration tests - fix `test_check_for_material_names` unit test - add helper function to app.py
ff248ff
to
6d62da9
Compare
OpenMCDepcode - add helper funtions to calculate neutronics parameters - ...
356005c
to
c483259
Compare
- add machinery in abc.py to preserve all depcode output - add machinery in app.py that allows fission_q to be set correctly - add machinery in openmc_msbr_model.py to make stochastic volume calculation optional - Move duplicated code in the `run_depletion_step` methods to Depcode.run_depletion_step - Make `convert_nuclide_code_to_name` a required function for all Depcode child classes - Streamline SaltProc-generated tallies for OpenMC coupled simulatiosn - add helper functions to ensure data consistency when storing results in the SaltProc results database
move serpent integration tests
c483259
to
d2c6957
Compare
6c591b6
to
786d14e
Compare
1d0cc21
to
72d02bc
Compare
f818230
to
d0f2241
Compare
Co-authored-by: Sam Dotson <44342873+samgdotson@users.noreply.github.com>
@samgdotson please allow other people some time to get their reviews in before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yardasol! thanks for getting this in. Overall it looks pretty good to me, I just have a couple of questions.
@@ -57,6 +57,7 @@ jobs: | |||
path: | | |||
/usr/share/miniconda3/envs/saltproc-env | |||
~/openmc_src | |||
~/mcpl_src |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is mcpl
? I'm not familiar with that, assuming that the directory name is for a code source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure, but it appears to be some sort of file format. I added MCPL to SaltProc's CI in #182, but I forgot to add full caching support, which I am doing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I just hadn't heard of it before and I was curious.
|
||
- name: Test SaltProc | ||
run: | | ||
pytest --ignore tests/integration_tests/run_no_reprocessing_serpent --ignore tests/integration_tests/run_no_reprocessing_openmc --ignore tests/integration_tests/run_constant_reprocessing_serpent --ignore tests/integration_tests/run_constant_reprocessing_openmc tests/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you ignoring all of these integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serpent ones require export controlled software, which I don't even know how I would get onto a GH actions runner, and the OpenMC ones take a long time to run, too long for CI in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I get not including the serpent tests. Is there a way to speed up the OpenMC tests? Also, what is even being tested in the CI if you include tests/
in the list of directories to ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second Amanda's question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to speed up the OpenMC tests?
I haven't been able to find a way to do this.
Also, what is even being tested in the CI if you include tests/ in the list of directories to ignore?
tests/
is not being ignored, only the directories that have the --ignore
flag are being ignored. You can look at the Checks tab to verify this.
examples/msbr/openmc_msbr_model.py
Outdated
@@ -51,15 +51,22 @@ def parse_arguments(): | |||
------- | |||
deplete : bool | |||
Flag indicated whether or not to run a depletion simulation. | |||
volume : bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
volume : bool | |
volume_calculation: bool |
This just feels like a more descriptive variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded, though I also agree with later comments that volume_calculation is very close to a function name, and something like stoch_volume or stoch_vol might work.
examples/msbr/openmc_msbr_model.py
Outdated
""" | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument('--deplete', | ||
type=bool, | ||
default=False, | ||
help='flag for running depletion') | ||
parser.add_argument('--volume', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parser.add_argument('--volume', | |
parser.add_argument('--volume_calculation', |
If you change it earlier the code
examples/msbr/openmc_msbr_model.py
Outdated
|
||
args = parser.parse_args() | ||
return bool(args.deplete) | ||
return bool(args.deplete), bool(args.volume) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
examples/msbr/openmc_msbr_model.py
Outdated
@@ -372,7 +379,7 @@ def plot_geometry(name, | |||
return plot | |||
|
|||
|
|||
deplete = parse_arguments() | |||
deplete, volume = parse_arguments() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
examples/msbr/openmc_msbr_model.py
Outdated
#msbr_volume_calc.set_trigger(1e-03, 'rel_err') | ||
settings.volume_calculations = [msbr_volume_calc] | ||
settings.run_mode = 'volume' | ||
if volume: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
if volume: | ||
msbr_volume_calc = openmc.VolumeCalculation([fuel, moder], int(1e10), ll, ur) | ||
#msbr_volume_calc.set_trigger(1e-03, 'rel_err') | ||
settings.volume_calculations = [msbr_volume_calc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe my suggestion wasn't that great because it's pretty similar to this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like stoch_volume
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or stoch_vol, if you're worried about length?
tallies = openmc.Tallies() | ||
tallies += list(delayed_lambda.tallies.values()) | ||
tallies += list(beta.tallies.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the structure of this object? Is is a giant list of the tallies? Are there labels for the tallies anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Tallies
object is a list of tallies with some additional class functions. Yes the tallies have their own IDs and names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you're summing various tallies into the same list? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,95 @@ | |||
"""Run SaltProc without reprocessing""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is later in the PR, but do you have a test file running SaltProc with reprocessing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few things I was curious about and some minor corrections, but overall looks good.
if volume: | ||
msbr_volume_calc = openmc.VolumeCalculation([fuel, moder], int(1e10), ll, ur) | ||
#msbr_volume_calc.set_trigger(1e-03, 'rel_err') | ||
settings.volume_calculations = [msbr_volume_calc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like stoch_volume
?
def convert_nuclide_code_to_name(self, nuc_code): | ||
"""Converts depcode nuclide code to symbolic nuclide name. | ||
|
||
Parameters | ||
---------- | ||
nuc_code : str | ||
Nuclide code | ||
|
||
Returns | ||
------- | ||
nuc_name : str | ||
Symbolic nuclide name (`Am242m1`). | ||
|
||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with abc
, what does this do?
ref_mats = openmc_depcode.read_depleted_materials(True) | ||
openmc_depcode.output_path = old_output_path | ||
|
||
openmc_depcode.update_depletable_materials(ref_mats, 12.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the 12.0
here?
Co-authored-by: LukeSeifert <44068471+LukeSeifert@users.noreply.github.com>
if entropy: | ||
entropy_mesh = openmc.RegularMesh() | ||
entropy_mesh.lower_left = ll | ||
entropy_mesh.upper_right = ur | ||
entropy_mesh.dimension = (20, 20, 20) | ||
settings.entropy_mesh = entropy_mesh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition!
saltproc/openmc_depcode.py
Outdated
@@ -312,7 +314,7 @@ def _create_mass_percents_dictionary(self, mat, percent_type='ao'): | |||
mat : openmc.Material | |||
A material | |||
percent_type : str | |||
Percent type of material | |||
Percent type of material, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental comma?
diluted_model = openmc.Model(materials=materials, geometry=geometry, settings=settings) | ||
reactions, diluted_materials = MicroXS._add_dilute_nuclides(self.chain_file_path, | ||
diluted_model, | ||
1e3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be helpful to include a variable inital_atom_dens = 1e3
or something similar, but if that won't need to be changed later then feel free to leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes, looks good to me. I'll wait for other people to get their reviews in before merging.
I'm fixing one of the local integration tests, so hold off on merging |
03529c2
to
3749522
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I think it looks good. My biggest comment is that there are some functions without any docstrings. Even if it's just a quick one line docstring explaining the purpose of the function/test, I think it would be helpful. I left a comment marking files where I noticed functions with missing docstrings.
|
||
- name: Test SaltProc | ||
run: | | ||
pytest --ignore tests/integration_tests/run_no_reprocessing_serpent --ignore tests/integration_tests/run_no_reprocessing_openmc --ignore tests/integration_tests/run_constant_reprocessing_serpent --ignore tests/integration_tests/run_constant_reprocessing_openmc tests/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second Amanda's question.
"sim_name": "msbr_openmc_test" | ||
}, | ||
"reactor": { | ||
"volume": 1.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the volume 1.0 here because the example is only examining a small portion of the salt?
examples/msbr/openmc_msbr_model.py
Outdated
@@ -51,15 +51,22 @@ def parse_arguments(): | |||
------- | |||
deplete : bool | |||
Flag indicated whether or not to run a depletion simulation. | |||
volume : bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded, though I also agree with later comments that volume_calculation is very close to a function name, and something like stoch_volume or stoch_vol might work.
examples/msbr/openmc_msbr_model.py
Outdated
|
||
args = parser.parse_args() | ||
return bool(args.deplete) | ||
return bool(args.deplete), bool(args.volume), bool(args.entropy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one, if you change variable name
examples/msbr/openmc_msbr_model.py
Outdated
@@ -372,7 +384,7 @@ def plot_geometry(name, | |||
return plot | |||
|
|||
|
|||
deplete = parse_arguments() | |||
deplete, volume, entropy = parse_arguments() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you change volume to something else
"efficiency": { | ||
"Kr": 0, | ||
"Xe": 0, | ||
"H": 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sparger has an efficiency of zero? Does that mean it's not removing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so.
"efficiency": { | ||
"Kr": 0.9115218695901359, | ||
"Xe": 0.9148573428740562 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is the gas separator where you account for removal, not the sparger? Does this separator not also remove the helium gas from the liquid salt before it hits a liquid pump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see, the entrainment separator does not remove helium. I think the way the reprocessing system files are set up is the least understood/documented aspect of SaltProc right now. I'm going to be adding some guides on this.
assert depcode_input['chain_file_path'] == \ | ||
str((input_path / 'test_chain.xml').resolve()) | ||
str((input_path / 'chain_endfb71_pwr.xml').resolve()) | ||
|
||
modified_operator_kwargs = expected_depletion_settings['operator_kwargs'].copy() | ||
if filename == 'constant_fission_yield': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings
assert openmc_depcode._convert_nucname_to_pyne('U238') == 'U238' | ||
assert openmc_depcode._convert_nucname_to_pyne('Ag110_m1') == 'Ag110M' | ||
assert openmc_depcode._convert_nucname_to_pyne('Am242') == 'Am242' | ||
assert openmc_depcode._convert_nucname_to_pyne('Am242_m1') == 'Am242M' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings
|
||
return list(self._fissile_nucs.union(self._fertile_nucs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the function name is descriptive enough. It gets the fissile and fertile nuclides.
@samgdotson @abachma2 @ZoeRichter @LukeSeifert the integration tests have been fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @yardasol. I can merge if others are happy.
This PR adds full support for OpenMC by implementing file and database IO functions in
OpenMCDepcode
.To my reviewers: Please do not panic at the large change size! Most of these are
.h5
files! You do not need to look at them!Note that for depletion simulations, the total mass density of the materials does not change in Serpent whereas in OpenMC it does.
Summary of changes
Specifically, this PR:
run_depletion_step()
method toDepcode
to reduce code repetition.Depcode
:convert_nuclide_code_to_name()
_convert_name_to_nuccode()
, which is implemented in bothSerpentDepcode
andOpenMCDepcode
preserve_simulation_files()
, which preserves all input and output files for each depletion step.app.py
app.py
to process thedepletion_settings
argument for OpenMC-coupled runs.SerpentDepcode
:resolve_include_paths()
, that converts relative paths to absolute paths ininclude
cards._convert_name_to_nuccode
, which is a helper function to assist in converting nuclide names to nuclide codes.OpenMCDepcode
:_check_for_material_names()
which fails fast if a material doesn't have a name.read_step_metadata
_find_xs_path()
, a helper function forread_step_metadata
read_neutronics_parameters()
_calculate_breeding_ratio()
, a helper function forread_neutronics_parameters()
_calculate_delayed_quantity()
a helper function forread_neutronics_parameters()
_get_values_with_uncertainties()
, a helper function for_calculate_delayed_quantity()
_calculate_fission_masses()
, a helper function forread_neutronics_parameters()
read_depleted_materials()
_create_mass_percents_dictionary()
, a helper function forread_depleted_materials()
_convert_nucname_to_pyne()
, a helper function for_create_mass_percents_dictionary()
_get_power_from_tallies()
, a helper function forread_depleted_materials()
convert_nuclide_code_to_name()
_convert_name_to_nuccode()
update_depletable_materials()
write_saltproc_openmc_tallies()
_get_fissile_fertile_nuclides()
, a helper function forwrite_saltproc_openmc_tallies()
Simulation._add_missing_nuclides()
, a function that enables expansion of previously stored results. We need this functionality due to the presence of decay-only nuclides, which are not always stored in the depletion matrix and thus the results.Types of changes
Required for Merging
Associated Issues and PRs
Associated Developers
Checklist for Reviewers
Reviewers should use this link to get to the
Review Checklist before they begin their review.