Skip to content
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

[ENH] Implement Results Reader #130

Merged
merged 26 commits into from
May 17, 2018
Merged

[ENH] Implement Results Reader #130

merged 26 commits into from
May 17, 2018

Conversation

DanKotlyar
Copy link
Contributor

The results reader parser, jupyter example notebook and unittest require review.

Attempt to remove the amount of files "changed" due to changing line endings. 
Automatically convert line endings to \n upon commit.
results.py - parser
test_ResultsReader - unittest
pwr_res.m, pwr_res_filter.m files used by the test
ResultsReader.ipynb - jupyter notebook
InnerAssembly_res.m - used by .ipynb file
@drewejohnson
Copy link
Collaborator

As for the travis build, the third cell (This notebook demonstrates .....) needs to be converted to Markdown so that it gets commented out when converted to python via testNotebooks.sh

"cell_type": "markdown",
"metadata": {},
"source": [
"# # Basic operations"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space here is what is blocking the subtitle formatting

]
},
{
"cell_type": "raw",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cell needs to be markdown, not raw

@drewejohnson drewejohnson mentioned this pull request May 9, 2018
@drewejohnson drewejohnson self-assigned this May 9, 2018
Most updated develop (#132) is merged into results-reader-3
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic improvement over the previous iteration! Largely, I think we need more test cases to cover all the possible uses of this file. More below

Things blocking approval

  1. Fix travis build
  2. More test cases - problem with no universes and problem with no time dependence
  3. Fewer minor attributes stored on ResultsReader object
  4. More commentary in jupyter notebook, rather than just blocks of code
  5. Convert jupyter notebook to rst and include in documentation
  6. Pre-compile regular expressions
  7. Merge latest changes from develop

More detail

Fewer attributes stored on reader

I think a lot of the methods you use to store positional data, varNames varValues, posFile, etc., are not needed and should be returned from their various methods. My reasoning is largely that we don't care about these values after the file has been read and they used very scarcely, most by a single method. These can then be passed to the various _storeX methods.

More test cases

I noticed that the reader seems intended for files with group constant data over time, and the tests largely reflect that. But this is not the entire scope of use for the results file. I would like to see two additional test files and test cases: one with no universes but with time dependence, and another with no universes nor time dependence. I left some comments on test_ResultsReader,py that should make writing the tests easier for you, since the execution of the tests is largely consistent across all test cases.

Pre-compiled regular expressions

There are a lot of regular expression calls here, and many for good reason. I believe we can speed this up by storing compiled regular expressions, i.e. re.compile(r'^\w+'), either outside the class, or as a class attribute,

class ResultsReader(---):
    _regexScalar = re.compile(----)

These can still call the search and match functions with
self._regexScalar.search(tline). This approach saves a little bit of time spent creating and destroying the regular expressions.

Documentation

The notebook looks good, but I get lost towards the end when vast amounts of code are presented with little to no commentary. These notebooks should be easy to understand for someone who is just picking up the ResultsReader. Furthermore, the notebook needs to be converted and added to documentation.

"""
Parser responsible for reading and working with result files.

.. note::

Branching points for unique burnup steps and universes,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this block and add more specific documentation on the results reader, such as the attributes

Attributes
------------
metadata: ...
univdata: ...

Rather than describe these variables via comments, describe them in the docstring.

Link to docstrings in developer guide

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attributes
------------
metadata: dict
    Dictionary with serpent descriptive variables as keys and the
    corresponding description. Within the _res.m file this data is
    printed multiple times, but contains the same description
    and thus is stored only once. e.g., 'version': 'Serpent 2.1.29'
resdata: dict
    Dictionary with serpent time-dependent variables as keys and the
    corresponding values. The data is unique for each burnup step.
    Some variables also contain uncertainties.
    e.g., 'absKeff': [[9.91938E-01, 0.00145],[1.81729E-01, 0.00240]]
univdata: dict
    Dictionary of universe names and their corresponding
    :py:class:`~serpentTools.objects.containers.HomogUniv`
    objects. The keys describe a unique state:
    'universe', burnup (MWd/kg), burnup index, time (days)
    ('0', 0.0, 1, 0.0)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly 👍 That block should be included in the ResultsReader docstring

path to the depletion file
path to the results file

Errors
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raises
---------
<Type of error that is raised>: <Why this error is raised>

Copy link
Contributor Author

@DanKotlyar DanKotlyar May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raises
------
SerpentToolsException:  Serpent version is not supported
                        No universes are found in the file
                        No results are collected
                        Corrupted results
IOError: file is unexpectedly closes while reading

XSReader.__init__(self, filePath, 'xs')
self.__fileObj = None
self._counter = {'meta': 0, 'rslt': 0, 'univ': 0}
self._univlist = list()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an empty list with []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to:
self._univlist = []

'inxs': 'INF_',
'b1xs': 'B1_'}
MapStrVersions = {}
for iVer in range(10, 31):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the intent here, but this should be stripped down. This creates 10 sub-dictionaries, potentially modifies one of them, and then only uses one of them depending upon the serpent version. Since we know what versions we support, and what version the file is from the user and the _precheck method, we can better create and utilize this dictionary

Copy link
Contributor Author

@DanKotlyar DanKotlyar May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A (nested) dictionary is now explicitly defined. The mapping is done for each version.

   MapStrVersions = {'2.1.29': {'meta': 'VERSION ', 'rslt': 'MIN_MACROXS', 'univ': 'GC_UNIVERSE_NAME',
                                 'days': 'BURN_DAYS', 'burn': 'BURNUP', 'inxs': 'INF_', 'b1xs': 'B1_'},
                      '2.1.30': {'meta': 'VERSION ', 'rslt': 'MEAN_SRC_WGT', 'univ': 'GC_UNIVERSE_NAME',
                                 'days': 'BURN_DAYS', 'burn': 'BURNUP', 'inxs': 'INF_', 'b1xs': 'B1_'}}

self._keysVersion = MapStrVersions[rc['serpentVersion']]
else:
raise SerpentToolsException("Version {} is not supported by the "
"ResultsReader".format(rc['serpentVersion']))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception should really be raised in the _precheck method, as well as ensuring that the version presented by the user agrees with that found in the input file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception was moved to the _precheck. In addition, SerpentToolsException is raised when the version defined in the setting is not matching the actual serpent version (obtained from the file).

if divmod(self._counter['meta'],self._counter['univ'])[0] != self._counter['rslt']:
debug(' found {} universes, {} time-points, and {} overall result points'.
format(self._counter['univ'], self._counter['rslt'], self._counter['meta']))
raise SerpentToolsException("The file {} is not complete ".format(self.filePath))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be raised if not universes are present in the file, but the file is in fact complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not changed, however the postcheck function was slightly modified:
if not self.resdata and not self.metadata and not self.univdata:
raise SerpentToolsException("No results were collected "
"from {}".format(self.filePath))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the debug information should be included in the exception to give information as to why we believe the file to be incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug info is now included in the exception.

        raise SerpentToolsException(
            "The file {} is not complete. The reader found {} universes, "
            "{} time-points, and {} overall result points ".format(self.filePath,
            self._counter['univ'], self._counter['rslt'], self._counter['meta']))

from serpentTools.messages import SerpentToolsException

class _ResultsTesterHelperFull(unittest.TestCase):
"""Test case to share setUpClass for testing Results Reader"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the docstrings and names of these helpers to reflect their purpose and scope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
Class with common tests for the results reader.

Expected failures/errors:

    1. test_varsMatchSettings:
                    compares the keys
                    defined by the user to those
                    obtained by the reader
        Raises SerpentToolsException
    2. test_metadata:
                    Check that metadata variables and
                    their values are properly stored
        Raises SerpentToolsException
    3. test_resdata:
                    Check that time-dependent results variables
                    and their values are properly stored
        Raises SerpentToolsException
    4. test_univdata:
                    Check that expected states are read
                    i.e., ('univ', bu, buIdx, days)
                    For a single state, check that
                    infExp keys and values are stored.
                    Check that infUnc and metadata are properly stored
        Raises SerpentToolsException
"""

badReader.read()
os.remove(badFile)

def test_variables(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what variables are being tested? Change the name to reflect the scope and effect of this test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def test_varsMatchSettings(self):

class ResultsTesterFull(_ResultsTesterHelperFull):
"""Class for testing the results reader."""

def test_raiseError(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what errors are being tested? Change the name to reflect the scope and effect of this test

@@ -0,0 +1,233 @@
"""Test the results reader."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment for these readers. The way the Testers are written, there is no need for the helpers. One specific helper is used to build one specific tester. Since the tests are largely the same, just the data compared is different, the actual tests should be put inside a single helper, with different subclasses that test a specific use case. Minimal example

class _Helper(unittest.TestCase):
  def test_1(self):
    <do test 1 using instance attributes>
  def test_2(self):
    <do test 2 using instance attributes>
...
class Case1Tester(_Helper):
  @classmethod
  def setupclass(cls):
    <get the reader using the specific settings>
    <set a variety of expected results to be tested against using methods on _Helper>
<repeat for all test cases, e.g. full file, filtered file, file with no universes, file with no time-dependence, etc>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

@DanKotlyar
Copy link
Contributor Author

Thanks for the comments and suggestions. I'll try to implement these quickly.
Regarding the universe related issues, I think that we can really rely on having this information simply since this is the essence of the results file.

@drewejohnson drewejohnson added this to the 0.5.0 milestone May 10, 2018
@drewejohnson
Copy link
Collaborator

Just ran a simple test problem with no explicit set gcu parameter using 2.1.30. Group constants for root universe were in the results file. I now agree that we can safely assume the results file will always have some universe data. A unit test for this case is no longer needed for approval

DanKotlyar and others added 11 commits May 11, 2018 08:26
all the files from examples and the doc/welcome/license
Run `git revert develop <>` on all images in docs/examples/images,
and on a few other files modified by commit
679e692
results.py
test_ResultsReader
pwr_res_noUniv.m for testing
test_ResultsReader.py includes 14 test cases.
parser is able to read with and without burnups
.ipynb is not ready
pwr_res_noBU.m was added for testing
…ues with settings.

Jupyter notebook has issues to upload all the settings.
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing the changes I requested. They look great!

Things blocking approval

  1. Fixing jupyter notebook - also part of Unable to import all settings from jupyter #140
  2. Ability to store uncertainties for items like CMM_DIFFCOEF and CMM_TRANSPXS
  3. Convert notebook to rst and include in documentation
  4. Document results reader in docs/api
  5. Use universes rather than univdata on reader
  6. Explicitly store expected data for testing as sets where appropriate
  7. Better way to extract universes

More Detail

CMM and non-metadata related XS

The way in which data is passed to HomogUniv objects is by checking if the variable name essentially begins with INF or B1. If so, we assume that the data has uncertainties and pass that to the universe. Otherwise, we don't assume there are uncertainties.

However, there are some group constants that don't start with those two strings that do contain uncertainties. This information needs to be passsed to the universe object.

Documentation

Once completed, the notebook needs to be converted to rst following the Examples guide in the dev docs. Also need to document the reader itself in docs/api following the guide in API - dev docs.

Also, the dictionary MapStrVersions seems very important and could require changes as we begin supporting new versions of SERPENT. Include some information in docs/develop/serpentVersions.rst under a new section in Version with New File Formats so that we can easily determine what we need to change for supporting previous results files

Better way to extract universes

Some of the universes have very long keys, that must be entered exactly in order to properly extract the universe. From the jupyter notebook, we can see one universe would require a key of ('3102', 0.10000000000000001, 2, 1.20048) to be obtained.

Implementing something like the BranchContainer.getUniv method can make it easier to extract a universe given only the universe ID and the index.

corresponding values. The data is unique for each burnup step.
Some variables also contain uncertainties.
e.g., 'absKeff': [[9.91938E-01, 0.00145],[1.81729E-01, 0.00240]]
univdata: dict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename univdata to universes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

No universes are found in the file
No results are collected
Corrupted results
IOError: file is unexpectedly closes while reading
"""

def __init__(self, filePath):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation behind removing the declaration of metadata, resdata, and univdata from the __init__ method? This behavior is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is included.
self.metadata, self.resdata, self.universes = {}, {}, {}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I was only looking at a partial diff when I made that. Great change

return
self._getVarValues(tline) # values to be stored
varType, varVals = self._getVarValues(tline) # values to be stored
if self._posFile == 'meta':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see self._posFile being used outside of this method. Can you have whereAmI return this value after performing all the checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        if self._posFile == 'meta':
            self._storeMetaData(varNamePy, varType, varVals)
        if self._posFile == 'rslt':
            self._storeResData(varNamePy, varVals)
        if self._posFile == 'univ':
            self._storeUnivData(varNameSer, varVals)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that the keyword is not found 'self._posFile' remains as it was. The function whereAmI either return the position or does not update the existing 'self._posFile'

=======

.. include:: ../../LICENSE
.. _license:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reset this file to develop. This may have been introduced by some of my pushed changes through 😬 I will look in to this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file was reset


def _read(self):
"""Read through the results file and store requested data."""
info('Preparing to read {}'.format(self.filePath))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this info calls. These are done automatically by the BaseReader.read method that, in turn, calls this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

self._storeUnivData(varNameSer, varVals)

def _whereAmI(self, tline):
"""Identify the position in the file."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can return posFile rather than store it on the object. It is only used in _processResults

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posFile is stored on the object simply because the method will not update the position, unless one of the keywords is found. This will lead to returning a non-existing variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have the method return None if none of the keywords are found.

What about the case where _whereAmI does not update posFile? That would indicate we are not at the beginning of a results, metadata, nor universe block, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The posFile will always be one of the these blocks: results, metadata, universes.
when I scroll through the file and find one of keywords, I update the posFile to its location.
Example:
if 'VERSION' is found I know that this is the metadata block and thus self._posFile will store this.
The next variable is still in the results block, however, the posFile will not be updated. Therefore, the function whereAmI will not change the posFile field.
The posFile will be changed once the results block is found.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. Then we do need to keep this as an attribute. Thanks for explaining!

self.__serpentVersion = rc['serpentVersion']
self._counter = {'meta': 0, 'rslt': 0, 'univ': 0}
self._univlist = []
self.metadata, self.resdata, self.univdata = {}, {}, {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename univdata to universes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.


def __init__(self, filePath):
XSReader.__init__(self, filePath, 'xs')
self.__fileObj = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following was deleted.
self.__fileObj = None

@@ -0,0 +1,233 @@
"""Test the results reader."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

@@ -0,0 +1,2515 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on the jupyter notebook:

  1. Additional commentary looks great.
  2. Use %matplotlib inline instead of notebook. If you navigate to how the image is shown here, nothing is displayed in the plotting window. I'm also not sure how the conversion to rst will work with notebook
  3. Copyright needs to be for all contributors, as they all helped build this project.
  4. Make code render in Markdown with single ticks, not apostrophes i.e.
'keys'
# not
`keys`'
  1. Why do we need the comparison in block [9], where we compare the version of SERPENT used to read the file and what is stored on the rc object? Is the fact that we were able to read a 2.1.30 file with 2.1.29 a good thing or a bad thing?
  2. Delete the additional call to %matplotlib notebook in block [16]
  3. Rename univdata calls in markdown cells to universes to reflect other changes in this review
  4. Can clean up the demo of the univdata section by storing a HomogUniv object with universe = res.universes['3102, 0.0, 1, 0.0] and then doing all the calls to sub-dictionary keys on this object
  5. We need to get Unable to import all settings from jupyter #140 closed before this notebook is complete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything was modified.
In respect to the version used vs. defined: a warning would be raised, but otherwise as long as we support the version, at attempt to read will be made.

All the files were modified to include this change:
results.py
test_ResultsReader.py
ResultReader.ipynb
The parser was update and now include .getUniv method. The unittest include 4 additional tests. Jupyter notebook is done. Missing: container for groupVals and groupUnc and also post check to test if universes field contains data or empty.
results.py includes an additional test that relies on the 'hasData' method inserted to containers.pt. 
test_ResultsReader was introduced with a test to check the new feature. Additional results file is needed for the new test: pwr_res_emptyAttributes.m.
Jupyter notebook includes the new copy rights.
must be given. The method allows to input more than one time parameter,
however only one is used to extract the data.
e.g., .getUniv('0',0.1, 1, 10), .getUniv('0',burnup=0.1), getUniv('0',timeDays=10)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is not needed. The sphinx autodoc will automatically create documentation for this based on the method docstring

Major change from univ.metadata to univ.gc, univ.gcUnc, univ.groups, univ.microGroups
Unittest tests all these changes. Notebook is updated accordingly.
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things blocking approval

  1. Changes to jupyter notebook [below]
  2. Converted notebook - rst for documentation
  3. Remove warning block in docs/api/results.rst
  4. Include this PR and a brief summary in docs/changelog.rst

Changes to jupyter notebook

  1. Remove block [2] where the rc keys are printed
  2. Explicitly set the serpent version to be 2.1.30 at the top of the file to remove the warning
  3. Remove the comparison in block [9] and the above markdown cell used to compare serpent version from setting and from file
  4. Remove the disclaimer about plotting not yet available. We don't know how "near term" this could be 😁
  5. Update the link in block [18] to point to the full HomogUniv object - http://serpent-tools.readthedocs.io/en/latest/api/containers.html#serpentTools.objects.containers.HomogUniv
  6. Remove mentions of metadata in comments for blocks 29 and 30

Parser, unittest, notebook and all the documentation is ready.
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! Nearly done 🏃‍♂️

Things blocking review

  1. Add code blocks to rst - this one is my fault due to poor explanation of what exactly needs to be changed in the dev docs. It's only the ipython3 string that needs to be removed
  2. Format the MapStrVersions in docs/develop/serpentVersions.rst using a code block

Things that would be absolutely fantastic but not needed

  1. In docs/examples/ResultsReader.rst, when you reference a class, such as the ResultsReader, an attribute, or a method, use the Sphinx python referencing system, example
``ResultsReader`` -> :py:class:`~serpentTools.parsers.results.ResultsReader`
``.resdata`` -> :py:attr:`~serpentTools.parsers.results.ResultsReader.resdata`

This is more conversion on your part, but you can use things like the replace directive to save typing. Check out the depletion reader example for some hints. Not required for review, but eventually we will convert these links.

- metadata: general description of the simulation
- resdata: time-dependent values, e.g. k-eff
- universes: universe dependent values, such as cross-sections
The mapping through the variable `MapStrVersions` should reflect this.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For rst, you have to use double ticks to get the code formatted properly - http://docutils.sourceforge.net/docs/ref/rst/roles.html#literal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- universes: universe dependent values, such as cross-sections
The mapping through the variable `MapStrVersions` should reflect this.

The basic mapping definition relies on the following structure:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a second colon here and tab the whole block in, like

.....structure::

    MapStrVersions = { ....

Further reading - http://www.sphinx-doc.org/en/stable/rest.html?highlight=url%20#source-code

This way the code will be rendered properly in the documentation. The empty line that separates the first line of code from the first line of text is crucial as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

should ease the analyses.



Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should clarify the developer guide more. Only the ipython3 phrase needs to be removed. The .. code:: has to remain or else this the blocks will not be rendered as code at all. This one's on me, and the dev guide will be updated accordingly

settings option.

A detailed description on how to use the settings can be found on:
http://serpent-tools.readthedocs.io/en/latest/api/settings.html
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Correct this in the notebook as well] Better link would be http://serpent-tools.readthedocs.io/en/latest/settingsTop.html, as this explains what all the settings are and how they influence the code

@drewejohnson drewejohnson changed the title Completed Results Reader [API] Implement Results Reader May 16, 2018
serpentVersion.rst was modified to include proper code in the documentation. ResultsReader.rst was modified to have proper documentation. Notebook was updated with a new reference to settings.
ResultsReader.rst in docs/examples was modified.
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results reader example looks phenomenal now. One final request to change the main heading for that file.
Since many of your sections have the same distinction (line of =) as your main header, sphinx takes them all to be chapter headings. I copied the rendered layout from docs/_build/html/examples/index.rst below

Results Reader

    Basic operations

Metadata (metadata)
Results Data (resdata)

    Plotting Results Data (resdata)

Universe Data (universes)

    Get Universe Data (.getUniv)
    Plotting universes
    User Defined Settings
    Conclusion
    References

The most minor of request, and then we're done here!


.. |metadata| replace:: :py:attr:`~serpentTools.parsers.results.ResultsReader.metadata`

.. |universes| replace:: :py:attr:`~serpentTools.parsers.results.ResultsReader.universes`

Results Reader
==============
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an empty line and an additional line of '=' over the 'Results Readerheading. This causes all other sections to be directly under this chapter. Some of the sections, likeMetadataare at the same level as this with only a single line of=. After building html with make clean html, if you navigate to docs/_build/html/examples/index.html`, then you can see how this affects the layout of this section

@drewejohnson drewejohnson changed the title [API] Implement Results Reader [ENH] Implement Results Reader May 17, 2018
The final change aligned the headings in the .rst file
Copy link
Collaborator

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🏆 🎉

@DanKotlyar DanKotlyar merged commit e4d30fc into develop May 17, 2018
@drewejohnson drewejohnson mentioned this pull request May 17, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants