Add OpenMC depletion settings to input file #177

yardasol · 2022-11-28T05:46:32Z

Summary of changes

This PR expands the inputfile JSON schema to enable specifying OpenMC depletion settings.
This PR also adds options to the inputfile JSON schema to specify timestep type (cumulative or stepwise) and units, as well as machinery to process these new inputs.
This PR also adds default values to the inputfile JSON schema, and a class to process these default values.

Specifically, this PR:

Adds, removes, or modifies the following input parameters:
- Depcode
  - exec_path: adds a default value
    - sss2 for codename == "serpent"
    - openmc_deplete.py for codename == "openmc"
  - num_depsteps -> n_depletion_steps
  - For codename == 'openmc':
    - (new) -> chain_file_path
    - (new) -> depletion_settings
- Simulation
  - db_name: adds a default value of saltproc_results.h5
  - restart_flag: adds a default value of false
  - adjust_geo: adds a default value of false
- Reactor
  - dep_step_length_cumulative -> depletion_timesteps
  - (new) -> timestep_units
  - (new) -> timestep_type
Removes input variables from input files that are equal to their defaults
Adds machinery to SerpentDepcode.set_power_load() to enable use of the new time step parameters.
Consistency changes to related files and tests

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)

Required for Merging

Associated Issues and PRs

Issue: closes Feature: Add machinery to specify depletion settings in the main input file. #176

Associated Developers

Dev: @

Checklist for Reviewers

Reviewers should use this link to get to the
Review Checklist before they begin their review.

…ion test

…e and tests

LukeSeifert

I had a few questions and pointed out a few typos I noticed, but overall looks good.

LukeSeifert · 2022-12-10T01:48:10Z

saltproc/_schema_default.py

@@ -0,0 +1,21 @@
+from jsonschema import Draft202012Validator, validators
+


Consider adding a comment here describing what this is used for

You mean a doc string for extend_with_default?

LukeSeifert · 2022-12-10T01:54:25Z

saltproc/app.py

@@ -12,6 +12,7 @@

 from saltproc import SerpentDepcode, OpenMCDepcode, Simulation, Reactor
 from saltproc import Process, Sparger, Separator, Materialflow
+from ._schema_default import DefaultValidatingValidator


How come this is ._schema_default instead of _schema_default?

I think this is because _schema_default is not a module. Maybe @munkm can chime in.

it just means you're importing from the current directory. However, if it's a member of saltproc you should be able to do from saltproc._schema_default import DefaultValidatingValidator

Separately, is there a more descriptive or less self-referential name for this function?

Yes, let me try DefaultFillingValidator

LukeSeifert · 2022-12-10T01:56:35Z

saltproc/app.py

            except jsonschema.exceptions.ValidationError:
                print("Your input file is improperly structured.\
                      Please see saltproc/tests/test.json for an example.")

+        j = obj


Why not continue using obj instead of switching back to j notation?

saltproc/app.py

saltproc/openmc_depcode.py

LukeSeifert · 2022-12-10T02:52:01Z

saltproc/openmc_deplete.py

@@ -63,12 +62,18 @@ def parse_arguments():
                           settings=settings,
                           tallies=tallies)

-with open(args.depletion_settings) as f:
+depletion_settings = {}


It doesn't look like this line does anything

It does. It initializes the depletion_settings dictionary. I think we need to do this so we can get the value provided by depletion_settings = json.load(f) inside the with block on line 66-67 in openmc_deplete.py, but maybe I'm wrong?

You definitely do not need to do this, you're just overwriting an empty dictionary with another dictionary.

Suggested change

depletion_settings = {}

The situation would be different if you were, say, combining dictionaries. But that doesn't appear to be the case.

LukeSeifert · 2022-12-10T03:05:04Z

tests/conftest.py

+    simulation = _create_simulation_object(object_input[1], depcode, 1, 1)
+        #db_path=str(
+        #    cwd /
+        #    'serpent_data' /
+        #    'tap_reference_db.h5'))
+
+    reactor = _create_reactor_object(object_input[2])


Consider adding extra variables i.e. simulation_object = object_input[1], that way its clearer what is being pulled from object_input

LukeSeifert · 2022-12-10T03:22:30Z

tests/unit_tests/test_simulation.py

+    old_db_path = simulation.db_path
+    simulation.db_path = str(Path(simulation.db_path).parents[1] / 'tap_reference_db.h5')
    assert simulation.read_k_eds_delta(7) is False
+    simulation.db_path = old_db_path


How come the db_path needs to be updated here?

For consistency if we ever add more tests to test_simulation.py.

…emas

…ared schemas" This reverts commit c1f8835.

yardasol · 2023-01-11T22:49:16Z

@abachma2 @munkm @samgdotson bump

LukeSeifert

Have a few things I wasn't certain about, but overall looks good.

LukeSeifert · 2023-01-16T16:46:29Z

doc/fileformatspec/simulation_input.rst

+    Restart simulation from the step when it stopped?
+
+  :type:
+    ``boolean``
+
+  :default:
+    ``false``
+
+
+.. _adjust_geo_property:
+
+``adjust_geo``
+--------------
+
+  :description:
+    switch to another geometry when keff drops below 1?


How come these are questions? Also, switch should be capitalized

They are questions that the variable is the answer to. Maybe that is confusing though?

LukeSeifert · 2023-01-16T16:47:39Z

saltproc/app.py

            try:
-                jsonschema.validate(instance=j, schema=v)
+                DefaultValidatingValidator(schema).validate(input_parameters)
+                #jsonschema.validate(instance=j, schema=v)


This can be removed

good catch/

LukeSeifert · 2023-01-16T16:51:28Z

saltproc/app.py

        else:
            raise ValueError(
-                f'{depcode_input["codename"]} '
-                'is not a supported depletion code')
+                f'{codename} is not a supported depletion code.'


I don't see codename defined in this function, thought I do see depcode_input["codename"]. Consider changing back to match rest of if statement, or adding in a codename= statement prior to the if statements

LukeSeifert · 2023-01-16T16:57:43Z

saltproc/app.py

+    if codename == 'openmc':
+        depletion_settings = depcode_input.pop('depletion_settings')
+        chain_file_path = depcode_input.pop('chain_file_path')
+
+    depcode = depcode(**depcode_input)
+
+    if codename == 'openmc':
+        depcode.chain_file_path = chain_file_path
+        depcode.depletion_settings = depletion_settings
+
+    depcode_input['codename'] = codename


How come this moved from handling OpenMC and Serpent to only OpenMC? Also, the separation of the if statements here seems a bit strange. Can this be rewritten into a single if statement? Consider adding a bit more information to the docstring

It has to do with the SaltProc input file. For Serpent runs, the depcode_input dictionary consists exclusively of valid keyword arguments for the SerpentDepcode.__init__() function. However, if we are doing an OpenMC run, there are additional key-value pairs in the depcode_input dictionary (depletion_settings and chain_file_path) that will cause an error if we run depcode(**depcode_input) without removing them first. However, I now realize what I really should do is just add these variables as optional keyword arguments to OpenMCDepcode.__init__().

LukeSeifert · 2023-01-16T17:00:22Z

saltproc/app.py

    Process SaltProc reactor class input parameters based on the value and
-    data type of the `num_depsteps` parameter, and throw errors if the input
+    data type of the `n_depletion_steps` parameter, and throw errors if the input
    parameters are incorrect.


Consider mentioning the added codename parameter in this docstring

saltproc/app.py

LukeSeifert · 2023-01-16T17:33:33Z

saltproc/input_schema.json

+                    "default": false}
+            },
+            "default": {},
+            "required": ["sim_name", "db_name"]


How come db_name is still required? It is given a default value

Good point.

samgdotson

Mostly cosmetic changes. However, there is one issue where the "cumulative" case is confusing.

samgdotson · 2023-01-17T19:02:24Z

saltproc/_schema_default.py

@@ -0,0 +1,21 @@
+from jsonschema import Draft202012Validator, validators
+


You mean a doc string for extend_with_default?

samgdotson · 2023-01-17T19:06:59Z

saltproc/app.py

@@ -12,6 +12,7 @@

 from saltproc import SerpentDepcode, OpenMCDepcode, Simulation, Reactor
 from saltproc import Process, Sparger, Separator, Materialflow
+from ._schema_default import DefaultValidatingValidator


it just means you're importing from the current directory. However, if it's a member of saltproc you should be able to do from saltproc._schema_default import DefaultValidatingValidator

Separately, is there a more descriptive or less self-referential name for this function?

samgdotson · 2023-01-17T19:08:00Z

saltproc/app.py

@@ -30,22 +34,22 @@ def run():
    simulation.check_restart()
    # Run sequence
    # Start sequence
-    for dep_step in range(len(msr.dep_step_length_cumulative)):
-        print("\n\n\nStep #%i has been started" % (dep_step + 1))
+    for step_idx in range(len(msr.depletion_timesteps)):


Is there a difference between the step index and the step number? I would just call it step. Though this is purely a stylistic difference.

I'm just trying to be as explicit as possible.

Okay, I don't see a difference but it doesn't hurt.

samgdotson · 2023-01-17T19:09:26Z

saltproc/app.py

        if depcode_input['codename'] == 'serpent':
            depcode_input['template_input_file_path'] = str((
                input_path /


Suggested change

if depcode_input['codename'] == 'serpent':

depcode_input['template_input_file_path'] = str((

input_path /

if codename == 'serpent':

depcode_input['template_input_file_path'] = str((

input_path /

samgdotson · 2023-01-17T19:09:28Z

saltproc/app.py


+        depcode_input['codename'] = depcode_input['codename'].lower()


Is this supposed to be

Suggested change

depcode_input['codename'] = depcode_input['codename'].lower()

codename = depcode_input['codename'].lower()

?

samgdotson · 2023-01-17T19:13:58Z

saltproc/app.py

+        ts = list(np.diff(depletion_timesteps))
+        depletion_timesteps = depletion_timesteps[0] + ts


Suggested change

ts = list(np.diff(depletion_timesteps))

depletion_timesteps = depletion_timesteps[0] + ts

depletion_timesteps = np.cumsum(depletion_timesteps)

I agree with @LukeSeifert that the expected result is unclear. Possibly use np.cumsum()?

This is actually coded improperly now that I look at it. What we are supposed to be doing here is converting a cumulative timeseries (1s, 3s, 5s) to a stepwise timeseries (1s, 2s, 2s). But I eneterd this wrong which explains why we were confused. I seem to be missing tests for this function as well so I'll add those

samgdotson · 2023-01-17T19:18:27Z

saltproc/openmc_deplete.py

@@ -63,12 +62,18 @@ def parse_arguments():
                           settings=settings,
                           tallies=tallies)

-with open(args.depletion_settings) as f:
+depletion_settings = {}


You definitely do not need to do this, you're just overwriting an empty dictionary with another dictionary.

Suggested change

depletion_settings = {}

The situation would be different if you were, say, combining dictionaries. But that doesn't appear to be the case.

samgdotson · 2023-01-17T19:22:18Z

tests/conftest.py

-def serpent_depcode(cwd):
-    """SerpentDepcode object for unit tests"""
+def serpent_runtime(cwd, tmpdir_factory):
+    """SaltProc objects for Serpent unit tests"""
    saltproc_input = str(cwd / 'serpent_data' / 'tap_input.json')
    _, _, _, object_input = read_main_input(saltproc_input)


We've discussed this before, I think. I don't like the practice of using underscores as variable names. They should have names, even if you're not using them. Also, consider that you might forget the return variables are for this function. Yes you could look it up, but why waste the energy?

Okay, I think I can address this in a way that satisfies both of us.

samgdotson · 2023-01-17T19:51:17Z

tests/conftest.py

+    simulation = _create_simulation_object(object_input[1], depcode, 1, 1)
+        #db_path=str(
+        #    cwd /
+        #    'serpent_data' /
+        #    'tap_reference_db.h5'))
+
+    reactor = _create_reactor_object(object_input[2])


LukeSeifert

Everything looks good to me.

@samgdotson

review of @samgdotson Co-authored-by: Sam Dotson <[email protected]>

yardasol · 2023-01-18T20:35:56Z

@samgdotson I made some changes based on your comments, and added some tests for additional code coverage. Let me know what you think

yardasol · 2023-01-19T18:18:55Z

Looping @LukeSeifert back in because I added some new stuff.

samgdotson

Thanks for making those other changes -- I'm curious about the nested functions.

samgdotson · 2023-01-19T18:28:01Z

saltproc/_schema_default.py

+def extend_with_default(validator_class):
+    validate_properties = validator_class.VALIDATORS["properties"]
+
+    def set_defaults(validator, properties, instance, schema):
+        for property, subschema in properties.items():
+            if "default" in subschema:
+                instance.setdefault(property, subschema["default"])
+
+        for error in validate_properties(
+            validator, properties, instance, schema,
+        ):
+            yield error
+
+    return validators.extend(
+        validator_class, {"properties" : set_defaults},
+    )


I didn't catch this before, but what is the purpose of having nested function definitions? Why not break this into two functions that can both be unit-tested?

I'm not sure. I pulled this block of code from here

@samgdotson I actually created an issue to look into this as there is a bug that exists with the current setup.

samgdotson · 2023-01-19T18:29:17Z

saltproc/app.py

@@ -30,22 +34,22 @@ def run():
    simulation.check_restart()
    # Run sequence
    # Start sequence
-    for dep_step in range(len(msr.dep_step_length_cumulative)):
-        print("\n\n\nStep #%i has been started" % (dep_step + 1))
+    for step_idx in range(len(msr.depletion_timesteps)):


Okay, I don't see a difference but it doesn't hurt.

samgdotson

lgtm

…epletion-settings Add OpenMC depletion settings to input file 5b4e60c

…mc-depletion-settings Add OpenMC depletion settings to input file 5b4e60c

yardasol force-pushed the openmc-depletion-settings branch from 806ff1a to 6896f02 Compare November 28, 2022 19:16

yardasol and others added 5 commits November 28, 2022 13:21

add machinery to set depletion settings for openmc in main input file

6896f02

fix json schema (format, defaults)

745c037

move default schema code to new file; fix missing param in Reactor

91b1e69

fix input schema

c663d58

fix file_interface_serpent integration test

6ec1362

yardasol mentioned this pull request Dec 5, 2022

Cleanup Depcode class #172

Merged

17 tasks

yardasol force-pushed the openmc-depletion-settings branch 2 times, most recently from bec77b4 to a33d3c9 Compare December 6, 2022 17:29

yardasol and others added 9 commits December 6, 2022 11:35

Merge branch 'master' into openmc-depletion-settings

a33d3c9

add reactor fixutres; cleanup timestep handling; fix failing integrat…

a854654

…ion test

move lowercasing 'codename' to its first needed case

68b210e

update releasenotes

ee6210b

remove cruft comments

a1b2447

cleanup _process_main_input_reactor_params()

dd6c8cc

add more coverage to test_read_main_input()

544f35e

remove input variables from input files that are equal to their defaults

434ac51

add default values for various parameters; consistency changes in cod…

d138aa5

…e and tests

yardasol force-pushed the openmc-depletion-settings branch from 9192e57 to d138aa5 Compare December 8, 2022 20:20

update releasenotes

be03b9d

yardasol requested review from samgdotson, abachma2, munkm and LukeSeifert and removed request for samgdotson December 9, 2022 18:31

yardasol marked this pull request as ready for review December 9, 2022 18:31

LukeSeifert reviewed Dec 10, 2022

View reviewed changes

yardasol and others added 2 commits January 9, 2023 14:27

Merge branch 'master' into openmc-depletion-settings

4b77fd2

typo fixes in input_schema.json

3bdc6d4

overhaul input file description in docpages

ffaf326

yardasol force-pushed the openmc-depletion-settings branch from 7b87265 to ffaf326 Compare January 10, 2023 22:16

yardasol added 3 commits January 11, 2023 12:37

add unevaluatedProperties keyword to limit json input to declared sch…

c1f8835

…emas

Revert "add unevaluatedProperties keyword to limit json input to decl…

3cc1a18

…ared schemas" This reverts commit c1f8835.

Add tests that check various cases for OpenMC depletion input

398cb13

yardasol mentioned this pull request Jan 11, 2023

[Bug]: Fix JSON Schema validation and add test coverage for all inputs #184

Open

yardasol requested a review from LukeSeifert January 11, 2023 22:48

changes from @LukeSeifert's review

9344619

yardasol force-pushed the openmc-depletion-settings branch from ca45926 to 9344619 Compare January 11, 2023 22:58

LukeSeifert reviewed Jan 16, 2023

View reviewed changes

yardasol mentioned this pull request Jan 17, 2023

Feature: Remove unneeded variables in input file #185

Open

yardasol requested a review from LukeSeifert January 17, 2023 19:22

samgdotson requested changes Jan 17, 2023

View reviewed changes

Changes from @LukeSeifert's second review

df31b93

yardasol force-pushed the openmc-depletion-settings branch from 0a95c29 to df31b93 Compare January 17, 2023 19:53

LukeSeifert approved these changes Jan 17, 2023

View reviewed changes

Add tests for timestep validating and scaling functions; changes from

80625ae

review of @samgdotson Co-authored-by: Sam Dotson <[email protected]>

yardasol requested a review from samgdotson January 18, 2023 17:42

update the endfb71 msbr serpent model to match the new syntax

1f1d8af

yardasol requested a review from LukeSeifert January 19, 2023 18:18

samgdotson requested changes Jan 19, 2023

View reviewed changes

yardasol requested a review from samgdotson January 19, 2023 20:02

samgdotson approved these changes Jan 19, 2023

View reviewed changes

samgdotson merged commit 5b4e60c into arfc:master Jan 19, 2023

github-actions bot pushed a commit that referenced this pull request Jan 19, 2023

update docs at commit: Merge pull request #177 from yardasol/openmc-d…

a0d8580

…epletion-settings Add OpenMC depletion settings to input file 5b4e60c

github-actions bot pushed a commit to khurrumsaleem/saltproc that referenced this pull request Jan 24, 2023

update docs at commit: Merge pull request arfc#177 from yardasol/open…

30b6148

…mc-depletion-settings Add OpenMC depletion settings to input file 5b4e60c

		@@ -0,0 +1,21 @@
		from jsonschema import Draft202012Validator, validators


		depcode_input['codename'] = depcode_input['codename'].lower()

	depcode_input['codename'] = depcode_input['codename'].lower()
	codename = depcode_input['codename'].lower()

		ts = list(np.diff(depletion_timesteps))
		depletion_timesteps = depletion_timesteps[0] + ts

	ts = list(np.diff(depletion_timesteps))
	depletion_timesteps = depletion_timesteps[0] + ts
	depletion_timesteps = np.cumsum(depletion_timesteps)

Add OpenMC depletion settings to input file #177

Add OpenMC depletion settings to input file #177

Conversation

yardasol commented Nov 28, 2022 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

LukeSeifert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Jan 11, 2023

LukeSeifert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samgdotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeSeifert left a comment

Choose a reason for hiding this comment

yardasol commented Jan 18, 2023

yardasol commented Jan 19, 2023

samgdotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samgdotson left a comment

Choose a reason for hiding this comment

yardasol commented Nov 28, 2022 •

edited

Loading

yardasol Jan 19, 2023 •

edited

Loading