Separate `depcode.py` into three files #174

yardasol · 2022-11-09T19:05:21Z

Summary of changes

The purpose of this PR is to make the changes in #172 easier to see for reviewers.

Specifically, this PR does the following:

Renames the depcode classes:
- DepcodeSerpent -> SerpentDepcode
- DepcodeOpenMC -> OpenMCDepcode
Separates the depcode.py file into three difference files:
- abc.py, storing Depcode
- openmc_depcode.py, storing OpenMCDepcode
- serpent_depcode.py, storing SerpentDepcode

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: Cleanup Depcode class #172

Associated Developers

Dev: @munkm

Checklist for Reviewers

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

abachma2

Hi @yardasol. Great work on this, I'm sure if took you some time to make sure you found all of the variable names that needed to updated. I have a few questions about some of the files. I will approve once you address my comments, although they are pretty minor.

abachma2 · 2022-11-09T19:13:02Z

doc/releasenotes/v0.5.0.rst

+  - ``DepcodeSerpent`` → ``SerpentDepcode``

    - ``template_inputfile_path`` → ``template_input_file_path``
    - Changed `iter_inputfile` and `iter_matfile` to be attributes instead of parameters


-  - ``DepcodeOpenmc`` is a ``Depcode`` subclass that interfaces with ``openmc``. This class implements the following functions 
+  - ``OpenMCDepcode`` is a ``Depcode`` subclass that interfaces with ``openmc``. This class implements the following functions 


Is there any reason you have one with the arrow and one without? Is OpenMCDepcode new? I thought you were renaming it.

OpenMCDepcode doesn't exist in v0.4.0 yet, so there's no need to indicate that the name has been changed inside the development period between v0.4.0 and v0.5.0.

Ok. That makes sense.

abachma2 · 2022-11-09T19:18:18Z

saltproc/abc.py

+from abc import ABC, abstractmethod
+
+class Depcode(ABC):


Couple things:

are you importing the ABC class from this module into this module?

Did you name this class ABC as in "abstract class"? Is this class ever seen by the user? If it is then this feels like a not-so intuitive class name for a user to have to remember. If the user doesn't see this, then disregard.

This pattern indicates that Depcode is an abstract class -- i.e. it inherits from the abc.ABC class. The purpose of abstract classes is to provide a template for future class creation. The expected usage is something like

MyClass(Depcode) def read_depcode_info(): do stuff

where MyClass overrides the read_depcode_info method (which doesn't currently do anything).

I get the Depcode being an abstract class part. My confusion is where the ABC class is coming from. Is ABC is being imported from the abc module into an abc module? If so, then is it another abc module this is being pulled from? Or is the abc module it's importing from a python package, like numpy or pandas?

He's importing from the latter. My suggestion was to rename the file to avoid this confusion.

I support this suggestion. Clearly I was confused by this.

I picked abc.py as it can hold many different classes that use abstract functions, but perhaps this isn't the best way of doing things.

If that's the case, maybe just write out abstract_classes.py?

abachma2 · 2022-11-09T19:21:21Z

saltproc/openmc_depcode.py

+        self.iter_inputfile = {'geometry': './geometry.xml',
+                               'settings': './settings.xml'},
+        self.iter_matfile = './materials.xml'


Do you mean to leave these paths hardcoded? Wouldn't you want to read in the values of the template_input_file dictionary?

I agree with this, it seems restrictive to have these hardcoded.

They are hardcoded because OpenMC by default expects that the settings, geometry, and material files are named settings.xml, geometry.xml materials.xml respectively. At the end of each reprocessing step, these files are the ones modified instead of the user's original OpenMC input files. The user has no need to specify a name for them.

So the user can specify files that they created from any location, then the code will copy them to have these names and be in this location. Then the code continuously calls these copied files and edits them as needed?

I get hardcoding the names, if that is what OpenMC requires. But would the file path ever change?

So the user can specify files that they created from any location, then the code will copy them to have these names and be in this location. Then the code continuously calls these copied files and edits them as needed?

Correct

I get hardcoding the names, if that is what OpenMC requires. But would the file path ever change?

The file path will only change if there's a change to OpenMC that requires us to change it as such.

abachma2 · 2022-11-09T19:21:49Z

saltproc/openmc_depcode.py

+    def read_depcode_info(self):
+        """Parses initial OpenMC simulation info from the OpenMC output files
+        and stores it in the `Depcode` object's ``sim_info`` attribute.
+        """


I'm assuming that most of the functions are placeholders for now, correct?

This actually confuses me, too. These are no longer @abstractmethods so shouldn't they do something?

These are placeholders and will be implemented in a future PR in the near future. Right now these methods don't actually do anything.

abachma2 · 2022-11-09T19:28:36Z

tests/integration_tests/run_no_reprocessing/test.py

@@ -46,7 +46,7 @@ def test_integration_2step_saltproc_no_reproc_heavy(setup):
    test_fuel_mdens = test_result['MAT_fuel_MDENS'][:, -1]

    test_mdens_error = np.array(ref_fuel_mdens - test_fuel_mdens)
-    np.testing.assert_array_equal(test_mdens_error, ref_mdens_error)
+    np.testing.assert_array_almost_equal(test_mdens_error, ref_mdens_error)


Why are you saying almost equal here? Is there some rounding error between values of the array here?

I use this because monte-carlo codes are stochastic and can have slightly different results each time you run them, which would cause a assert_equal statement to fail.

So this test is based on some result out of Serpent or OpenMC then? If that's the case, can you specify a random seed? Or does doing that still have some small variation in the result?

There can still be variation introduced by differences in hardware, compilers, etc.

The default sensitivity of assert_almost_equal is on the order of 10^-6, which I think is sufficiently small since we are comparing errors, not base values.

That makes sense. And this test passes?

samgdotson

Good job @yardasol. There are some minor-ish changes. In particular I think your file names could be clearer. abc and app are not useful indicators of their contents.

samgdotson · 2022-11-09T19:25:34Z

saltproc/abc.py

@@ -0,0 +1,163 @@
+from abc import ABC, abstractmethod


This file should be renamed to avoid a potential namespace conflict with abc. Also so that it's clearer to users what lives here.

Be sure to use git mv abc.py some-other-name.py to minimize the number of files in the commit.

samgdotson · 2022-11-09T19:38:14Z

saltproc/abc.py

+from abc import ABC, abstractmethod
+
+class Depcode(ABC):


This pattern indicates that Depcode is an abstract class -- i.e. it inherits from the abc.ABC class. The purpose of abstract classes is to provide a template for future class creation. The expected usage is something like

MyClass(Depcode) def read_depcode_info(): do stuff

where MyClass overrides the read_depcode_info method (which doesn't currently do anything).

samgdotson · 2022-11-09T19:39:03Z

saltproc/app.py

@@ -240,9 +240,9 @@ def _create_depcode_object(depcode_input):
    """Helper function for `run()` """
    codename = depcode_input['codename']
    if codename == 'serpent':
-        depcode = DepcodeSerpent
+        depcode = SerpentDepcode


I guess this is fine, lol. Why rearrange?

Personal preference, as well as consistency with how we would refer to these in natural language (i,e, Serpent depletion code corresponds better to SerpentDepcode than DepcodeSerpent)

samgdotson · 2022-11-09T19:39:30Z

saltproc/openmc_depcode.py

+from saltproc import Materialflow
+from saltproc.abc import Depcode
+
+class OpenMCDepcode(Depcode):


@abachma2 example of Depcode 😄

samgdotson · 2022-11-09T19:41:33Z

saltproc/openmc_depcode.py

+    def read_depcode_info(self):
+        """Parses initial OpenMC simulation info from the OpenMC output files
+        and stores it in the `Depcode` object's ``sim_info`` attribute.
+        """


This actually confuses me, too. These are no longer @abstractmethods so shouldn't they do something?

samgdotson · 2022-11-09T19:43:53Z

tests/conftest.py

-def depcode_openmc(cwd):
-    """DepcodeOpenMC object for unit tests"""
+def openmc_depcode(cwd):
+    """OpenMCDepcode object for unit tests"""
    saltproc_input = (cwd / 'openmc_data' / 'tap_input.json').as_posix()
    _, _, _, object_input = read_main_input(saltproc_input)


I don't love this... I get that the first several variables are unused -- but I think they should still have proper names. I don't think there are any performance benefits to this either. This is just a preference though, so if you have a compelling reason to use underscores go ahead, I suppose.

My perspective is that they are not relevant to anything else in the function, and giving them a null-like name indicates that. Maybe @munkm has some perspective on this?

samgdotson · 2022-11-09T19:45:31Z

saltproc/abc.py

+        self.iter_inputfile = './iter_input'
+        self.iter_matfile = './iter_mat'


Are these files created dynamically?

Yes, and their names will be inter_input and iter_mat respectively. They are hardcoded because they don't really need to be seen by the user.

samgdotson · 2022-11-09T19:46:37Z

saltproc/openmc_depcode.py

+        self.iter_inputfile = {'geometry': './geometry.xml',
+                               'settings': './settings.xml'},
+        self.iter_matfile = './materials.xml'


I agree with this, it seems restrictive to have these hardcoded.

samgdotson · 2022-11-09T19:49:46Z

saltproc/openmc_depcode.py

+            subprocess.check_output(
+                args,
+                cwd=os.path.split(self.template_inputfile_path)[0],
+                stderr=subprocess.STDOUT)


Just an FYI using subprocess.STDOUT can create some challenges for users on different platforms.

I had problems with it this summer when using watts. watts-dev/watts#52.

Namely users with

WSL2

windows

Thanks for the heads up. I'll look into fixing this as part of a PR that addresses #173

yardasol · 2022-11-09T20:35:49Z

@samgdotson I agree we should rename app.py, but not in this PR. @munkm Since the purpose of this PR is to get the file splitting changes out of the way, is it okay if I rename abc.py in #172 just so that we can avoid any nasty merge conflicts?

abachma2 · 2022-11-09T20:44:57Z

@samgdotson I agree we should rename app.py, but not in this PR. Is it okay if I rename abc.py in #172 just so that we can avoid any nasty merge conflicts?

I am okay with putting this into a different PR. I see that renaming the file is on the To Do list for that PR, and I like the name you suggest there in for abc.py. I do worry that #172 will be very large though, so please be aware of its size. Feel free to break it up into a few PRs.

yardasol · 2022-11-10T16:13:27Z

@samgdotson @abachma2 Is this good to be merged?

abachma2

I am approving this PR. I won't merge though until after @samgdotson gives his approval as well.

samgdotson

I guess this is fine if the issues raised will be resolved later.

…-depcode Separate `depcode.py` into three files 2cf2788

…rate-depcode Separate `depcode.py` into three files 2cf2788

yardasol added 3 commits November 3, 2022 14:57

DepcodeSerpent->SerpentDepcode; DepcodeOpenMC->OpenMCDepcode

43201a5

split up depcode.py into three files

acc6e21

use array_almost_equal

5edaf3a

yardasol added Comp:Core Status:5-In Review Priority:1-Critical Difficulty:1-Beginner labels Nov 9, 2022

yardasol marked this pull request as ready for review November 9, 2022 19:08

yardasol requested review from samgdotson, abachma2, LukeSeifert and munkm November 9, 2022 19:08

abachma2 reviewed Nov 9, 2022

View reviewed changes

samgdotson requested changes Nov 9, 2022

View reviewed changes

abachma2 approved these changes Nov 10, 2022

View reviewed changes

samgdotson self-requested a review November 10, 2022 20:32

samgdotson approved these changes Nov 10, 2022

View reviewed changes

samgdotson merged commit 2cf2788 into arfc:master Nov 10, 2022

github-actions bot pushed a commit that referenced this pull request Nov 10, 2022

update docs at commit: Merge pull request #174 from yardasol/separate…

0384d3b

…-depcode Separate `depcode.py` into three files 2cf2788

github-actions bot pushed a commit to khurrumsaleem/saltproc that referenced this pull request Nov 11, 2022

update docs at commit: Merge pull request arfc#174 from yardasol/sepa…

5da3d22

…rate-depcode Separate `depcode.py` into three files 2cf2788

github-actions bot pushed a commit to yardasol/saltproc that referenced this pull request Nov 11, 2022

update docs at commit: Merge pull request arfc#174 from yardasol/sepa…

3334963

…rate-depcode Separate `depcode.py` into three files 2cf2788

yardasol mentioned this pull request Dec 7, 2022

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

Closed

yardasol removed the Status:5-In Review label Mar 7, 2023

		self.iter_inputfile = './iter_input'
		self.iter_matfile = './iter_mat'

Separate depcode.py into three files #174

Separate depcode.py into three files #174

Conversation

yardasol commented Nov 9, 2022 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

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

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 Nov 9, 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

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

yardasol Nov 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Nov 9, 2022 • edited Loading

abachma2 commented Nov 9, 2022

yardasol commented Nov 10, 2022

abachma2 left a comment

Choose a reason for hiding this comment

samgdotson left a comment

Choose a reason for hiding this comment

Separate `depcode.py` into three files #174

Separate `depcode.py` into three files #174

yardasol commented Nov 9, 2022 •

edited

Loading

yardasol Nov 9, 2022 •

edited

Loading

yardasol Nov 9, 2022 •

edited

Loading

yardasol commented Nov 9, 2022 •

edited

Loading