Cleanup `Depcode` class #172

yardasol · 2022-11-03T20:08:54Z

Summary of changes

This PR makes several changes and improvements to the Depcode family of classes with the goal of improving code and documentation readability, as well as making future development and use easier by tweaking the program flow.

Specifically, this PR does the following:

Cleans up the docstrings and makes them consistent (TODO)
Makes it so the runtime files are put in the data storage directory instead of the working directory
Makes the following changes to the following methods and attributes:
- Remove junk default paths from depcode class parameters
- Makes npop, active_cycles, inactive_cycles Depcode class attributes and removes them from the input schema.
- Add some default values to the input schema
- Depcode
  - read_depcode_info() -> read_step_metadata()
  - sim_info -> step_metadata
  - read_depcode_step_param() -> read_neutronics_parameters()
  - param -> neutronics_parameters
  - read_dep_comp() -> read_depleted_materials()
  - run_depcode() -> run_depletion_step()
  - write_depcode_input() -> write_runtime_input()
  - write_mat_file() -> update_depletable_materials()
  - iter_inputfile -> runtime_inputfile
  - iter_matfile -> runtime_matfile
- SerpentDepcode
  - read_depcode_info() -> read_step_metadata()
  - sim_info -> step_metadata
  - read_depcode_step_param() -> read_neutronics_parameters()
  - param -> neutronics_parameters
  - read_dep_comp() -> read_depleted_materials()
  - run_depcode() -> run_depletion_step()
  - write_mat_file() -> update_depletable_materials()
  - create_nuclide_name_map_zam_to_serpent() -> map_nuclide_code_zam_to_serpent()
  - convert_nuclide_name_serpent_to_zam() -> convert_nuclide_code_to_zam()
  - get_nuc_name() -> convert_nuclide_code_to_name()
  - ~~change_sim_par() -> apply_neutron_settings()~~ remove change_sim_par()
  - added get_neutron_settings()
  - create_iter_matfile() -> create_runtime_matfile()
  - replace_burnup_parameters() -> set_power_load()
  - write_depcode_input() -> write_runtime_input()
  - cleanup switch_to_next_geometry()
  - cleanup insert_path_to_geometry()
  - iter_inputfile -> runtime_inputfile
  - iter_matfile -> runtime_matfile
- OpenMCDepcode
  - read_depcode_info() -> read_step_metadata()
  - sim_info -> step_metadata
  - read_depcode_step_param() -> read_neutronics_parameters()
  - param -> neutronics_parameters
  - read_dep_comp() -> read_depleted_materials()
  - run_depcode() -> run_depletion_step()
  - write_mat_file() -> update_depletable_materials()
  - write_depcode_input() -> write_runtime_input()
  - iter_inputfile -> runtime_inputfile
  - iter_matfile -> runtime_matfile
Consistency changes in test files and documentation to reflect new class, function, and variable names:

The following changes will be made in a PR after this one is merged:

moves abc.py to abstract_depcode.py (TODO)
rearrange function order (TODO)

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: Cleanup depcode.py and related files #162

Associated Developers

Dev: @

Checklist for Reviewers

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

consistency changes associated with this name changes

…eutronics_parameters conistency changes associated with function and attribute name change

associated changes to relevant files

…erpent() - variable name adjustments in function - consitency changes in relevant test files - docstring changes

- adjust docstrings - associated changes in other files

- docstring changes - associated testfile changes

- get_nuc_name() -> convert_nuclide_code_to_name() - convert_nuclide_name_serpent_to_zam() -> convert_nuclide_code_to_zam() - docstring cleanup and fixes - consitency changes in other relevant files

…file()

samgdotson

There are a few changes that I made as suggestions. This PR is 90+% renaming variables. The one change I really wanted was for saltproc/abc.py to be renamed to something else.

samgdotson · 2022-11-30T17:42:20Z

saltproc/app.py

@@ -247,12 +240,10 @@ def _create_depcode_object(depcode_input):
        raise ValueError(
            f'{depcode_input["codename"]} is not a supported depletion code')


Suggested change

f'{depcode_input["codename"]} is not a supported depletion code')

f'{codename} is not a supported depletion code. Accepts: "serpent" or "openmc".')

I can't make a suggestion for code that isn't modified in this PR, but you should use (on line 234)

codename = depcode_input["codename"].lower() # instead of codename = depcode_input["codename"]

That way your code isn't case sensitive.

I believe this gets handled by JSON Schema verification.

The code suggestion in the first comment is still valid. Forcing lower doesn't add any overhead really, but if you're confident then okay.

samgdotson · 2022-11-30T17:47:10Z

saltproc/input_schema.json

                "geo_file_paths": {
                    "description": "Path(s) to geometry file(s) to switch to in depletion code runs",
                    "type": "array",
                    "items": { "type": "string"},
                    "minItems": 1,
-                    "uniqueItems": false
+                    "uniqueItems": true


Why the change?

My rationale was that we will not want to switch to a geometry that we've already cycled through, but that may not actually be the case; we could 'oscillate' between a set of particular geometries. I'll change this back to false

samgdotson · 2022-11-30T17:50:11Z

saltproc/openmc_depcode.py

+            {'geometry': (output_path / 'geometry.xml').resolve().as_posix(),
+             'settings': (output_path / 'settings.xml').resolve().as_posix()}
+        self.runtime_matfile = (output_path / 'materials.xml').resolve().as_posix()


Does it have to be posix? What if a user has a Windows machine? I think .resolve() automatically provides the correct format.

Suggested change

{'geometry': (output_path / 'geometry.xml').resolve().as_posix(),

'settings': (output_path / 'settings.xml').resolve().as_posix()}

self.runtime_matfile = (output_path / 'materials.xml').resolve().as_posix()

{'geometry': (output_path / 'geometry.xml').resolve(),

'settings': (output_path / 'settings.xml').resolve()}

self.runtime_matfile = (output_path / 'materials.xml').resolve()

I wanted the paths as strings. I didn't think about what would happen if a user was on windows. I'll change this to str()

Has this been changed? It's not showing up for me

saltproc/openmc_depcode.py

samgdotson · 2022-11-30T17:54:12Z

saltproc/openmc_deplete.py

+    parser.add_argument('--materials',
                        type=str,
                        default=1,
                        help='path to openmc material \
                        material xml file')
-    parser.add_argument('-geo',
+    parser.add_argument('--geometry',
                        type=str,
                        default=1,
                        help='path to openmc geometry \
                        xml file')
-    parser.add_argument('-set',
+    parser.add_argument('--settings',
                        type=str,
                        default=None,
                        help='path to openmc settings \
                        xml file')
-    parser.add_argument('-tal',
+    parser.add_argument('--tallies',
                        type=str,
                        default=None,
                        help='path to openmc tallies \
                        xml file')
-    parser.add_argument('-dep',
+    parser.add_argument('--depletion_settings',
                        type=str,
                        default=None,
                        help='path to saltproc depletion \
                        settings  json file')


Can you create single-letter flags? I agree that it's clearer but you're making the CLI more cumbersome.

This CLI is not for the user. It is used internally for running openmc depletion via a script.

yardasol · 2022-12-02T21:05:48Z

The one change I really wanted was for saltproc/abc.py to be renamed to something else.

I will do this in a future PR, otherwise the changes to abc.py will be obscured by the name change.

abachma2

Hi @yardasol, great work on this PR. I agree with Sam that the vast majority of this PR is documentation and function names. Things seem to be consistent across your code, and the changes do improve the readability of the documentation. I am really curious as to the benefit of making the npop, active_cycles, and inactive_cycles attributes instead of in the input schema.

The two changes I requested are very small and are regarding the release notes.

abachma2 · 2022-12-05T16:41:44Z

examples/msbr/msbr_main.json

@@ -1,16 +1,13 @@
 {
   "proc_input_file": "msbr_objects.json",
   "dot_input_file": "msbr.dot",
-   "output_path": "./data",
+   "output_path": "data",


Any reason for this change? Is it because you've added the output path as an input parameter to the Depcode class?

This change just cleans up the path itself. The ./ is not necessary.

abachma2 · 2022-12-05T17:15:14Z

saltproc/serpent_depcode.py

+            Nuclide code in Serpent2 format (`47310.09c`)

        Returns
        -------
        nuc_name : str
-            Name of nuclide in human-readable notation (`Am-242m1`).
-        nuc_zzaaam : str
-            Name of nuclide in `zzaaam` form (`952421`).
+            Symbolic nuclide name (`Am242m1`).


Are you using the nuclide code for Am-242m1? Just want to make sure you're being consistent.

This is correct.

Ok. That's just a very unexpected nuclide code for Am-242m1

The 3 in 47310 is Serpent's convention for metastable nuclides:

The library directory includes two entries for each nuclide, one using the standard MCNP convention (ZA.id) and another one using the element symbol and the isotope mass (e.g. 92235.03c and U-235.03c for U-235). Either name can be used to identify the nuclide in the material compositions.

The script assumes that nuclides in isomeric states are identified by setting the third digit in the ZA to 3 (e.g. 61348 for Pm-148m or 95342 for Am-242m). If other convention is used, the isomeric state number (5. entry) must be set manually.

abachma2 · 2022-12-05T17:21:53Z

saltproc/serpent_depcode.py

+        file_lines.insert(line_idx,  # Insert on 9th line
+                          'set    power   %5.9E   dep daystep   %7.5E\n' %
+                          (current_power, step_length))


Are these values hardcoded into the file lines? Is there any reason someone would need to use different values?

The set power card is the only way to specify depletion steps in Serpent to my knowledge. Right now, we can only use the daystep option, but in #177 I fix this to enable use of every potential time unit a user might want to use.

That makes sense, but what about the 5.9E and 7.5E values in there? Are those values, or a way to read in a variable value?

This is string formatting. The 5 in 5.9E indicates we want to display a maximum of 5 digits before the decimal, the 9 indicates we want to display 9 digits after the decimal, and the E indicates we display the number in scientific notation:

>>> '%5.9f' % (2.0e-2) '0.020000000' >>> '%5.9E' % (2.0e-2) '2.000000000E-02'

abachma2 · 2022-12-05T17:36:57Z

tests/unit_tests/test_serpent_depcode.py

+    assert nuc_code_map[380880] == '38088.09c'
+    assert nuc_code_map[962400] == '96240.09c'
+    assert nuc_code_map[952421] == '95342.09c'
+    assert nuc_code_map[340831] == '340831'
+    assert nuc_code_map[300732] == '300732'
+    assert nuc_code_map[511262] == '511262'
+    assert nuc_code_map[420931] == '420931'
+    assert nuc_code_map[410911] == '410911'


Just curious. Is there any reason you chose to test these nuclides?

These are holdovers from Andrei's tests.

doc/releasenotes/v0.5.0.rst

Co-authored-by: Amanda Bachmann <amandab7@illinois.edu>

yardasol · 2022-12-05T22:05:05Z

@samgdotson bump

Co-authored-by: Amanda Bachmann <amandab7@illinois.edu>

samgdotson

There are still some changes to be addressed. In particular everywhere that forces a posix path. You're losing one of the key benefits of pathlib!

samgdotson · 2022-12-05T22:14:57Z

saltproc/openmc_depcode.py

+            {'geometry': (output_path / 'geometry.xml').resolve().as_posix(),
+             'settings': (output_path / 'settings.xml').resolve().as_posix()}
+        self.runtime_matfile = (output_path / 'materials.xml').resolve().as_posix()


Has this been changed? It's not showing up for me

samgdotson · 2022-12-05T22:15:48Z

saltproc/serpent_depcode.py

+        file_lines.insert(line_idx,  # Insert on 9th line
+                          'set    power   %5.9E   dep daystep   %7.5E\n' %
+                          (current_power, step_length))


samgdotson · 2022-12-05T22:17:09Z

tests/conftest.py

-    depcode.iter_inputfile = openmc_iter_inputfiles
-    depcode.iter_matfile = (openmc_input_path / 'materials.xml').as_posix()
+    depcode.runtime_inputfile = openmc_runtime_inputfiles
+    depcode.runtime_matfile = (openmc_input_path / 'materials.xml').as_posix()


I didn't catch this before, but this should also consider non-unix operating systems.

Suggested change

depcode.runtime_matfile = (openmc_input_path / 'materials.xml').as_posix()

depcode.runtime_matfile = str(openmc_input_path / 'materials.xml')

samgdotson · 2022-12-05T22:17:24Z

tests/conftest.py

@@ -28,7 +28,7 @@ def serpent_depcode(cwd):
    saltproc_input = (cwd / 'serpent_data' / 'tap_input.json').as_posix()
    _, _, _, object_input = read_main_input(saltproc_input)
    depcode = _create_depcode_object(object_input[0])
-    depcode.iter_inputfile = (cwd / 'serpent_data' / 'tap_reference').as_posix()
+    depcode.runtime_inputfile = (cwd / 'serpent_data' / 'tap_reference').as_posix()


Suggested change

depcode.runtime_inputfile = (cwd / 'serpent_data' / 'tap_reference').as_posix()

depcode.runtime_inputfile = str(cwd / 'serpent_data' / 'tap_reference')

samgdotson · 2022-12-05T22:17:45Z

tests/conftest.py

-            (openmc_input_path / openmc_iter_inputfiles[key]).as_posix()
+    for key in openmc_runtime_inputfiles:
+        openmc_runtime_inputfiles[key] = \
+            (openmc_input_path / openmc_runtime_inputfiles[key]).as_posix()


Suggested change

(openmc_input_path / openmc_runtime_inputfiles[key]).as_posix()

str(openmc_input_path / openmc_runtime_inputfiles[key])

samgdotson · 2022-12-05T22:19:28Z

tests/integration_tests/file_interface_openmc/test.py

        j = json.load(f)
        assert j['directory'] == Path(
-            openmc_depcode.iter_inputfile['settings']).parents[0].as_posix()
+            openmc_depcode.runtime_inputfile['settings']).parents[0].as_posix()


Can't suggest on non edited lines, but posix...

yardasol · 2022-12-05T22:40:48Z

There are still some changes to be addressed. In particular everywhere that forces a posix path. You're losing one of the key benefits of pathlib!

That's weird, I fixed this in this commit. Try looking at it again?

Co-authored-by: Sam Dotson <sgd2@illinois.edu>

samgdotson

Thanks for making changes

…depcode Cleanup `Depcode` class 5b6bf0c

yardasol force-pushed the cleanup-depcode branch 2 times, most recently from b2a849b to fc06882 Compare November 7, 2022 17:14

read_depcode_info() -> read_step_metadata(); sim_info -> step_metadata

4ea7e4f

consistency changes associated with this name changes

yardasol force-pushed the cleanup-depcode branch from fc06882 to 4ea7e4f Compare November 7, 2022 17:23

yardasol added 3 commits November 7, 2022 11:41

update depcode unit test filenames

152806f

read_depcode_step_param() -> read_neutronics_parameters(); param -> n…

4947cd7

…eutronics_parameters conistency changes associated with function and attribute name change

read_dep_comp() -> read_depleted_materials(); variable name adjustments

94b2647

associated changes to relevant files

yardasol force-pushed the cleanup-depcode branch from 4a82500 to 94b2647 Compare November 7, 2022 19:23

yardasol added 2 commits November 7, 2022 13:55

create_nuclide_name_map_zam_to_serpent() -> map_nuclide_code_zam_to_s…

053b64a

…erpent() - variable name adjustments in function - consitency changes in relevant test files - docstring changes

run_depcode() -> run_depletion_step()

93c0a1c

- adjust docstrings - associated changes in other files

yardasol force-pushed the cleanup-depcode branch 2 times, most recently from ed7b8fb to 0568fc6 Compare November 7, 2022 21:16

write_mat_file() -> update_depletable_materials()

d823203

- docstring changes - associated testfile changes

yardasol force-pushed the cleanup-depcode branch from 0568fc6 to d823203 Compare November 9, 2022 16:31

cleanup nuclide code handling functions

94f1479

- get_nuc_name() -> convert_nuclide_code_to_name() - convert_nuclide_name_serpent_to_zam() -> convert_nuclide_code_to_zam() - docstring cleanup and fixes - consitency changes in other relevant files

yardasol mentioned this pull request Nov 9, 2022

Separate depcode.py into three files #174

Merged

17 tasks

Merge remote-tracking branch 'upstream/master' into cleanup-depcode

d4f19e8

yardasol force-pushed the cleanup-depcode branch from a388626 to 475ff1f Compare November 26, 2022 17:57

yardasol added 7 commits November 26, 2022 12:02

change_sim_par() -> apply_neutron_settings(); cleanup read_plaintext_…

475ff1f

…file()

create_iter_matfile() -> create_runtime_matfile()

9ab1823

replace_burnup_parameters() -> set_power_load()

abc61f3

cleanup switch_to_next_geometry()

69ee50d

cleanup insert_path_to_geometry()

be59037

write_depcode_input() -> write_depletion_step_input()

f5aec4e

attribute name change: iter_* -> runtime_*

87fed81

yardasol mentioned this pull request Nov 27, 2022

Update Depcode unit test filenames #175

Merged

14 tasks

yardasol force-pushed the cleanup-depcode branch from 3ae72f1 to 5d98386 Compare November 27, 2022 20:02

write_depletion_step_input() -> write_runtime_input()

5d98386

yardasol force-pushed the cleanup-depcode branch from f23bc9b to 0d6aff4 Compare November 27, 2022 22:16

remove default path values in depcode classes

0d6aff4

fix integration tests

ac78eec

yardasol marked this pull request as ready for review November 29, 2022 22:37

Add test for get_neutron_settings()

4a38449

yardasol requested review from samgdotson and LukeSeifert November 30, 2022 16:45

Merge remote-tracking branch 'upstream/master' into cleanup-depcode

2cb688a

yardasol force-pushed the cleanup-depcode branch from 9c9a426 to 2cb688a Compare November 30, 2022 16:59

yardasol requested review from abachma2, ZoeRichter and munkm November 30, 2022 17:39

samgdotson requested changes Nov 30, 2022

View reviewed changes

yardasol added 2 commits November 30, 2022 14:53

Path.as_posix() -> str(Path)

4db68bf

set uniqueItems=false for geo_file_paths

660e88f

yardasol requested a review from samgdotson November 30, 2022 20:56

fix typo in file_interface_openmc

8396c2b

abachma2 approved these changes Dec 5, 2022

View reviewed changes

Apply suggestions from @abachma2 code review

4a49c11

Co-authored-by: Amanda Bachmann <amandab7@illinois.edu>

yardasol requested a review from abachma2 December 5, 2022 19:42

Apply suggestions from @abachma2 code review

ae1a740

Co-authored-by: Amanda Bachmann <amandab7@illinois.edu>

samgdotson requested changes Dec 5, 2022

View reviewed changes

Add @samgdotson suggestions.

647a1d1

Co-authored-by: Sam Dotson <sgd2@illinois.edu>

yardasol force-pushed the cleanup-depcode branch from 9db670d to 647a1d1 Compare December 5, 2022 22:44

yardasol requested a review from samgdotson December 5, 2022 23:02

samgdotson approved these changes Dec 6, 2022

View reviewed changes

samgdotson merged commit 5b6bf0c into arfc:master Dec 6, 2022

github-actions bot pushed a commit that referenced this pull request Dec 6, 2022

update docs at commit: Merge pull request #172 from yardasol/cleanup-…

5be14c7

…depcode Cleanup `Depcode` class 5b6bf0c

yardasol mentioned this pull request Dec 7, 2022

Feature: Reorder Depcode functions and make appropriate functions private #180

Closed

		@@ -247,12 +240,10 @@ def _create_depcode_object(depcode_input):
		raise ValueError(
		f'{depcode_input["codename"]} is not a supported depletion code')

	f'{depcode_input["codename"]} is not a supported depletion code')
	f'{codename} is not a supported depletion code. Accepts: "serpent" or "openmc".')

	depcode.runtime_matfile = (openmc_input_path / 'materials.xml').as_posix()
	depcode.runtime_matfile = str(openmc_input_path / 'materials.xml')

	depcode.runtime_inputfile = (cwd / 'serpent_data' / 'tap_reference').as_posix()
	depcode.runtime_inputfile = str(cwd / 'serpent_data' / 'tap_reference')

	(openmc_input_path / openmc_runtime_inputfiles[key]).as_posix()
	str(openmc_input_path / openmc_runtime_inputfiles[key])

Cleanup Depcode class #172

Cleanup Depcode class #172

Conversation

yardasol commented Nov 3, 2022 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

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

yardasol commented Dec 2, 2022

abachma2 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

abachma2 Dec 5, 2022 • edited Loading

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 Dec 5, 2022

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

yardasol commented Dec 5, 2022 • edited Loading

samgdotson left a comment

Choose a reason for hiding this comment

Cleanup `Depcode` class #172

Cleanup `Depcode` class #172

yardasol commented Nov 3, 2022 •

edited

Loading

abachma2 Dec 5, 2022 •

edited

Loading

yardasol commented Dec 5, 2022 •

edited

Loading