Homogenized Universe #35

sallustius · 2017-10-17T13:59:19Z

Container for storing and getting data.
Closes #11

Nothing to add

drewejohnson

Great work @sallustius! 🎆
Two big comments, and a few minor ones

The implementation of the get method needs to be corrected. As the comments indicate, the value of x is not guaranteed to always be the expected value, regardless of the uncertainty given. Secondly, if the uncertainty is True, then the arguments x and dx returned will be identical.
Will need to add some unit tests demonstrating that we can load data correctly, retrieve data correctly, and that errors are raised when things go wrong (i.e. requesting data that does not exist)
Docstrings need a little tweaking to match the numpy style that we are going with.
Some minor cleanliness errors that can likely be fixed by instructing pycharm to reformat the code (Ctrl+Shift+L, I believe). Things like

Yes:  x, y = ('foo', 'bar')
No: x,  y=('foo','bar' )

If you run pycodestyle, the linter should detect these, and ones that you did not cause.

drewejohnson · 2017-10-17T16:46:26Z

serpentTools/objects/containers.py

+
+
+class HomogUniv(_SupportingObject):
+    """Class for:


Docstring

Great detailed docstring! 👍 Few comments:

The first line of the docstring should be a brief example of the purpose this object fulfills

Add the parameters for the __init__ method in a Parameters block, labeled with type and description

You don't have to add the methods here, but if you do, they should also have a brief description

Here is a like to the numpy example for classes

Class items

We should probably add a __str__ method here [and in all our objects for that matter] that indicates the following information:

Type of container (class)

Name of the universe

Burnup and step to which this universe corresponds (add units of MWd/kgHM)

If given, the day as well

I add the if given statement, because it's not always guaranteed that the branching reader will have knowledge of the day, as the burnup and step are, by default, included and day is excluded. I think we should add the day argument as optional

Done only the first part as discussed

The __str__ issue will be addressed in the process of closing #36

drewejohnson · 2017-10-17T16:49:35Z

serpentTools/objects/containers.py

+    """
+
+    def __init__(self, container, name, bu, step, day):
+        """ Class Initializer. Each universe is defined  uniquely


Typically, private methods don't need docstrings, unless they are super confusing. The required input parameters can be listed in the class docstring above, of the manner

Parameters ------------ x: type <description on x> y: str or list or None or whatever <description on y>

drewejohnson · 2017-10-17T16:51:28Z

serpentTools/objects/containers.py

+        self.infUnc = {}
+
+    def set(self, variablename, variablevalue, uncertainty=False):
+        """


For multiline docstrings, practice is to leave the first line empty, and add a single line declarative statement describing the method/object. Further sections can be added below.
See above comment for documenting parameters.

Maybe include a Raises section in the docstring indicating that a warning message is raised if a variable is overwritten 🤷‍♂️

drewejohnson · 2017-10-17T16:52:14Z

serpentTools/objects/containers.py

+        # 1. Check the input type
+        variablename = _SupportingObject._convertVariableName(variablename)
+        if not isinstance(uncertainty, bool):
+            raise messages.error("Uncertainty must be a boolean variable")


Great! Add an indication of what type of variable was passed in to this method to make the error more helpful

drewejohnson · 2017-10-17T16:52:36Z

serpentTools/objects/containers.py

+        setter[variablename] = variablevalue
+
+    def get(self, variablename, uncertainty=False):
+


Add a docstring. This should include a Returns section indicating the data types of variables to be returned

drewejohnson · 2017-10-17T16:53:42Z

serpentTools/objects/containers.py

+        if not uncertainty:
+            return x
+        else:
+            dx = setter.get(variablename)


Here, x and dx will be the same item, as both are taken from the same setter dictionary. Will need to grab the appropriate uncertainty dictionary as well

drewejohnson · 2017-10-17T16:55:24Z

serpentTools/objects/containers.py

+        # 2. Pointer to the proper dictionary
+        setter = self._lookup(variablename, uncertainty)
+        # 3. Return the value of the variable
+        x = setter.get(variablename)


This does not ensure that x is always the expected value. If uncertainty is True, then x will actually be what dx should be. We need to guaruntee that, regardless of the value of uncertainty, x is the expected value from the correct inf/b1 dictionary.

drewejohnson · 2017-10-17T17:07:13Z

serpentTools/objects/containers.py

+            dx = setter.get(variablename)
+            return x, dx
+
+    def _lookup(self,variablename, uncertainty):


Single space after items in lists/tuples/functions/etc e.g.
_lookup(self, variableName, uncertainty):
Mixed case variables as well

drewejohnson · 2017-10-17T17:17:41Z

serpentTools/objects/containers.py

+            else:
+                return self.b1Unc
+
+        messages.error('Neither inf, nor b1 in the string')


Two items here.

Like the above error with the bad uncertainty type, the name of the variable should be passed into this error. This way, the user knows what our program is interpreting.

Last line in the file should be blank

drewejohnson · 2017-10-17T17:17:48Z

serpentTools/objects/containers.py

@@ -0,0 +1,92 @@
+import numpy


The numpy module isn't used here, so this line can be removed.

Further, could you add a brief module level docstring at the top of this file? It largely just needs to indicate the purpose of the file and classes present (to be expanded as we go)

This reverts commit d562209.

drewejohnson

Thank you for addressing the previous comments. I added a few small things relating to our previous conversations about storing non-xs related data (group structure, for example) on the object.

Unit tests are still required, but this file is nearly complete

drewejohnson · 2017-10-20T15:41:04Z

serpentTools/objects/containers.py

+results reader to store data for a single homogenized universe at a single
+instance in time.
+"""
+
 from serpentTools.objects import _SupportingObject


From #40, there is no longer a preceeding _ in the SupportingObject class

drewejohnson · 2017-10-20T15:47:02Z

serpentTools/objects/containers.py

-        """ Class Initializer. Each universe is defined  uniquely
-        in terms of the attributes mentioned in the docstring. The input
-        container refers to the name of the parser (branching/results reader).
-        """
        _SupportingObject.__init__(self, container)


Actually, since we have a name, use the NamedObject, and pass the name and the container into the __init__ here. Removes the need to also write self.name = name 😜

drewejohnson · 2017-10-20T16:04:04Z

serpentTools/objects/containers.py

        # 3. Check if variable is already present. Then set the variable.
-        if variablename in setter:
+        if variableName in setter:
            messages.warning('The variable will be overwritten')


Add the name of the variable to this line

drewejohnson · 2017-10-20T16:06:01Z

serpentTools/objects/containers.py

        # 1. Check the input values
-        variablename = _SupportingObject._convertVariableName(variablename)
+        variableName = _SupportingObject._convertVariableName(variableName)


Please remove.
This is only necessary if the variable name requested is SERPENT_STYLE. We will make it known to the user that the variables are stored with mixedCase names.

drewejohnson · 2017-10-20T16:06:47Z

serpentTools/objects/containers.py

            if not uncertainty:
                return self.b1Exp
            else:
                return self.b1Unc

-        messages.error('Neither inf, nor b1 in the string')
+        messages.error('%s not found.The string must contain either the   ...'
+                       'inf, or b1 substring.', variableName)


Option to add other data like group structure to the object?

gridley · 2017-11-08T21:33:49Z

Python doesn't have any private data, so why are you guys using setters and getters?

drewejohnson

Nearly done! Some changes to the errors calls (using TypeErrors vs. messages.error) and renaming the test methods to indicate what they are testing

drewejohnson · 2017-11-14T15:20:01Z

serpentTools/objects/containers.py

+        """
+        # 1. Check the input values
+        if not isinstance(uncertainty, bool):
+            raise messages.error('The variable uncertainty has type %s.\n ...'


Have this raise a TypeError with the same message instead. Same with the call in the addData method

drewejohnson · 2017-11-14T15:20:49Z

serpentTools/tests/test_branching.py

@@ -11,7 +11,7 @@


 class BranchTester(unittest.TestCase):
-    """Classs for testing the branching coefficient file reader."""
+    """Class for testing the branching coefficient file reader."""


drewejohnson · 2017-11-14T15:21:34Z

serpentTools/tests/test_container.py

+from serpentTools.parsers import DepletionReader
+
+
+class containerTester(unittest.TestCase):


Change the name of this class to reflect the container being tested. We have/will have many containers down the road

Done. Changed in "HomogenizedUniverseTester"

drewejohnson · 2017-11-14T15:23:03Z

serpentTools/tests/test_container.py

+
+    @classmethod
+    def setUpClass(cls):
+        container = DepletionReader(None)


Pedantic/non-crucial request: Can you just pass DepletionReader(None) as the first argument to containers.HomogUniv? There isn't a need to store the Depletion reader after the creation of the HomogUniv

drewejohnson · 2017-11-14T15:24:16Z

serpentTools/tests/test_container.py

+        for kk in cls.Unc:
+            cls.univ.addData(kk, cls.Unc[kk], True)
+
+    def test_1(self):


Rename the test to indicate the purpose of this test, i.e. test_getB1Exp

drewejohnson · 2017-11-14T15:24:34Z

serpentTools/tests/test_container.py

+            d[kk] = self.univ.get(kk,False)
+        self.assertDictEqual(self.b1Exp,d)
+
+    def test_2(self):


Same as ⬆️

drewejohnson · 2017-11-14T15:24:40Z

serpentTools/tests/test_container.py

+            d[kk] = self.univ.get(kk,True)
+        self.assertDictEqual(self.b1Unc,d)
+
+    def test_3(self):


Same as ⬆️

drewejohnson · 2017-11-14T15:24:44Z

serpentTools/tests/test_container.py

+            d[kk] = self.univ.get(kk, False)
+        self.assertDictEqual(self.infExp, d)
+
+    def test_4(self):


Same as ⬆️

drewejohnson · 2017-11-14T15:24:52Z

serpentTools/tests/test_container.py

+            d[kk] = self.univ.get(kk, True)
+        self.assertDictEqual(self.infUnc, d)
+
+    def test_5(self):


Same as ⬆️

drewejohnson

🎉 🎆 🎉 ✔️

Incorporated changes from PR #35 so that the variables are stored in the homogenized universe containers. Since the variables requested in reader.settings['variables'] may or may not exactly appear in the SERPENT output, a handy method _checkAddVariable was added to XSReader class. This will perform the following checks to see if a variable should be stored: No variables requested -> True; variable directly appears in requested variables -> True; User has requested that B1XS be stored and variable appears without B1_ -> True; User has requested that infinite xs be stored and variable appears without INF_ -> True; Otherwise, return False. This check is needed because we don't store ALL of the variable names in our settings dictionary, because there are a lot of double counts (INF_TOT and B1_TOT) Also added testing for the branching reader Signed-off-by: Andrew Johnson <1drew.e.johnson@gmail.com>

* Updated gitignore to exclude pycharm project settings * Skeleton files for each reader class; Basic directory setup * Depletion reader and project settings (#1) * Quick: DepletedMaterial returns time vector if not given as input (#2) * Quick fix: DepletedMaterial getXY only returns time if no time is given as input * Fixes #3; Cleaned up reader subclasses (#4) * Fixes #3; Cleaned up reader subclasses * Added versioneer (#18) * Fix get xy (#21) * Fixes #19: Material doesn't return days if given as input * Fixed unit tests * Versioneer (#23) * Messages, logging, and exception (#24) * Expand variable groups; XSReader base; Temporary settings (#27) * Branching testing; Temporary settings modification * Initial documentation - sphinx (#28) * Added variables.yaml to data_files in setup (#32) * Added depreciation and future warning decorators; More debug statements (#34) * Depletion reader now correctly adds data from TOT; Updated defaults (#39) * Better support (#40) * Added python 2.7 support (#41) * Homogenized Universe (#35) * Read depmtx (#44) * Return A matrix from depmtx as csc matrix not csr (#46) * Remove settings package; Cleaned up messages file (#47) * Implemented a module level read function (#48) * Examples (#49) * depmtx reader now parses using regular expressions (#52) * Updated documentation - developer guide, api, and examples (#59) * Updated setup.py to begrudgingly support distutils (#61) * Use warnings.catch_warnings to record warnings in test_settings (#55) * Native drewtils (#62) * Branching Reader (#65) * Release 0.1.0

* Release 0.1.0 (#66) * Updated gitignore to exclude pycharm project settings * Skeleton files for each reader class; Basic directory setup * Depletion reader and project settings (#1) * Quick: DepletedMaterial returns time vector if not given as input (#2) * Quick fix: DepletedMaterial getXY only returns time if no time is given as input * Fixes #3; Cleaned up reader subclasses (#4) * Fixes #3; Cleaned up reader subclasses * Added versioneer (#18) * Fix get xy (#21) * Fixes #19: Material doesn't return days if given as input * Fixed unit tests * Versioneer (#23) * Messages, logging, and exception (#24) * Expand variable groups; XSReader base; Temporary settings (#27) * Branching testing; Temporary settings modification * Initial documentation - sphinx (#28) * Added variables.yaml to data_files in setup (#32) * Added depreciation and future warning decorators; More debug statements (#34) * Depletion reader now correctly adds data from TOT; Updated defaults (#39) * Better support (#40) * Added python 2.7 support (#41) * Homogenized Universe (#35) * Read depmtx (#44) * Return A matrix from depmtx as csc matrix not csr (#46) * Remove settings package; Cleaned up messages file (#47) * Implemented a module level read function (#48) * Examples (#49) * depmtx reader now parses using regular expressions (#52) * Updated documentation - developer guide, api, and examples (#59) * Updated setup.py to begrudgingly support distutils (#61) * Use warnings.catch_warnings to record warnings in test_settings (#55) * Native drewtils (#62) * Branching Reader (#65) * Release 0.1.0

sallustius added 6 commits September 29, 2017 12:57

Just started

cce7967

Nothing to add

Merge branch 'develop' into homog_universe_ste

de57036

Updated container for result and branching parsers

0de7835

Merge branch 'develop' into homog_universe_ste

6566262

Corrected names and addition of warning messages

d89cf01

Cleaned code

55f13c2

sallustius requested a review from drewejohnson October 17, 2017 13:59

drewejohnson requested changes Oct 17, 2017

View reviewed changes

drewejohnson added enhancement parent support labels Oct 17, 2017

drewejohnson added this to the Version 1.0.0 milestone Oct 17, 2017

drewejohnson self-assigned this Oct 17, 2017

sallustius added 5 commits October 18, 2017 11:46

Requested corrections have been made

7313d62

Required changes

d562209

Revert "Required changes"

502d1a3

This reverts commit d562209.

Merge branch 'develop' into homog_universe_ste

3e420cf

Eliminated underscore

7401a84

drewejohnson requested changes Oct 20, 2017

View reviewed changes

sallustius added 2 commits November 13, 2017 16:54

Unit test on homogenized universe

b122e00

Merge branch 'develop' into homog_universe_ste

bd7d954

drewejohnson requested changes Nov 14, 2017

View reviewed changes

Requested modifications were carried out

95f4e64

drewejohnson approved these changes Nov 14, 2017

View reviewed changes

drewejohnson merged commit 06b5e92 into develop Nov 14, 2017

drewejohnson mentioned this pull request Nov 14, 2017

Support: Homogenized Universe Object #11

Closed

drewejohnson deleted the homog_universe_ste branch February 23, 2018 13:43

		setter[variablename] = variablevalue

		def get(self, variablename, uncertainty=False):

		from serpentTools.parsers import DepletionReader


		class containerTester(unittest.TestCase):

Homogenized Universe #35

Homogenized Universe #35

Conversation

sallustius commented Oct 17, 2017

drewejohnson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Docstring

Class items

sallustius Oct 18, 2017 • 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

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

drewejohnson 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

gridley commented Nov 8, 2017

drewejohnson 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

Choose a reason for hiding this comment

drewejohnson left a comment

Choose a reason for hiding this comment

drewejohnson left a comment •

edited

Loading

sallustius Oct 18, 2017 •

edited

Loading