Adds namelist and streams file interface #27

pwolfram · 2016-10-06T14:41:29Z

This is a prototype reader / writer for interfacing with namelist and streams and this will ultimately be needed to support generalization identified in #20.

A list of preliminary features (included or could be included):

Read-only status for the classes which build the reader/writer namelist and streams objects
Pure-python implementation that does not require use of command line tools, e.g., awk or sed and calls to the shell.
Type conversion, especially for things like numbers, times, and logic

pwolfram · 2016-10-06T14:45:22Z

@xylar and @milenaveneziani, this is the start of a set of classes which we can used to read / write namelist and streams files. At this point it is probably "prototype" quality code. Please let me know what you think. I'm thinking we should use this as a "straw-man" to build out general capability to manipulate namelist and streams files.

I'm putting this out here to stimulate discussion and as a starting point for the changes we need to generalize the code and fully expect many or all of the lines in this file to be rewritten or adapted to our needs.

pwolfram · 2016-10-06T14:53:50Z

@xylar and @milenaveneziani, the thing we need to focus on here is the API for the classes that interface with the namelist and streams files, e.g.,

# get check if global stats is on
nl = Namelist(nlistpath)
dt = nl.read('config_AM_globalStats_enable')

# get name for mesh file
sf = XMLList(streamspath)
meshname = sf('mesh', 'filename_template')

Once we have a good handle on this we should be able to write the necessary functionality that we need.

milenaveneziani · 2016-10-06T16:34:18Z

@pwolfram: this sounds good to me. How do you suggest we should test it? With an example script, or by modifying one of the scripts that we already have to do plotting/analysis?

pwolfram · 2016-10-06T17:51:33Z

@milenaveneziani, this brings up the large question of having unit tests. We could use pytest for that and essentially ensure that different parts of the code are doing precisely what they need to do to meet our requirements. There would be a new test folder that contains these unit tests to ensure that the code is working properly.

pwolfram · 2016-10-11T15:58:39Z

@milenaveneziani, I've pushed some changes that include unit tests via the pytest framework (similar to what xarray uses). Basically when you are in the folder you can type pytest to run the unit tests. I believe you'll need to conda install pytest to use this testing framework. Essentially we can use this to build out the key unit tests we need in the model.

pwolfram · 2016-10-11T16:03:44Z

Note @xylar and @milenaveneziani, this is still somewhat rough but one end goal here is to get automatic testing for each PR via pytest to ensure that we don't accidental break functionality that is important as we modify the repo. We will likely want to modify the interfaces uses for the namelist and streams reader / writer.

milenaveneziani · 2016-10-12T21:05:18Z

mpas_analysis/shared/io/namelist_streams_interface.py

@@ -0,0 +1,91 @@
+#!/usr/bin/python


Should be /usr/bin/env python

milenaveneziani · 2016-10-12T21:28:46Z

mpas_analysis/test/test_namelist_streams_interface.py

+10/07/2016
+"""
+
+import os


Is this needed?

milenaveneziani · 2016-10-12T21:29:30Z

mpas_analysis/test/test_namelist_streams_interface.py

+"""
+
+import os
+import pytest


Is this needed?

milenaveneziani · 2016-10-12T21:30:27Z

mpas_analysis/test/test_namelist_streams_interface.py

+        self.setup_namelist(readonly=True)
+        with self.assertRaisesRegexp(AssertionError, 'Cannot write to namelist file .* because readonly=True'):
+            self.nl.write('config_dt', '00:00:00')
+


@requires_lxml needed?

xylar · 2016-10-15T16:09:05Z

mpas_analysis/shared/io/namelist_streams_interface.py

+class XMLList:
+    """
+    Class to read in streams configuration file, provdies
+    read and write functionality


I'm not sure I understand why we would want write functionality as part of this repo. Can you suggest a case where writing or modifying a streams file might make sense?

xylar · 2016-10-15T16:10:11Z

mpas_analysis/shared/io/namelist_streams_interface.py

+        #print command
+        result = call(command,shell=True)
+
+class XMLList:


I think the class name is too general for the specific usage (or perhaps the description of the class is too specific)

xylar · 2016-10-15T16:10:56Z

mpas_analysis/shared/io/namelist_streams_interface.py

+            self.write(self.fname+'.backup')
+
+    def read(self, streamname, attribname):
+        """ name is a list of name entries terminanting in some value


Could you fix the docstring? Makes no sense so me currently.

xylar · 2016-10-15T16:19:36Z

mpas_analysis/shared/io/namelist_streams_interface.py

+        self.readonly = readonly
+        self.xmlfile = etree.parse(fname)
+        self.root = self.xmlfile.getroot()
+        if backup:


If you do decide to keep write/modify functionality, I think backup=True only makes sense if readonly=False.

xylar · 2016-10-15T16:21:01Z

mpas_analysis/shared/io/namelist_streams_interface.py

+                else:
+                    print "%s was not changed to %s because it didn't exist and we aren't setting new fields!"%(attribname, value)
+
+    def write(self, fname=None):


It seems odd to me that write doesn't check if readonly==True, especially if fname=None or fname==self.fname.

xylar · 2016-10-15T16:26:44Z

mpas_analysis/shared/io/namelist_streams_interface.py

+
+    def read(self, name):
+        # shell return value
+        return_val = check_output(['awk', '/'+name+'/{printf $3}', self.fname])


As you mentioned in the PR description, it would be preferable to have pure python. I would think it would make more sense to read in the full file on init and create a dictionary from the names and values. Then, the various get functions (get, getint, getfloat, getbool) that I suggest we use instead of read would would just return the dictionary value, possibly with the appropriate casting.

xylar · 2016-10-15T16:28:11Z

mpas_analysis/shared/io/namelist_streams_interface.py

+            return_val = return_val.strip('"').strip("'")
+        return return_val
+
+    def write(self, name, value):


I don't think we want namelist write functionality in this repo. @pwolfram, can you give me an example of where we might need this in this repo?

@xylar, we may want to use this type of code to make edits for namelists / streams for automatic test cases. This was the context that this code was original written to address. I think there is an advantage to having this functionality, even if we don't plan to immediately use it because we want the O part of IO too in order to make this a general tool. For example, I can easily envision using MPAS-Analysis to setup/analyze test cases in the testing core and this would be useful in this endeavor.

xylar · 2016-10-15T16:28:58Z

@pwolfram wrote:

the thing we need to focus on here is the API for the classes that interface with the namelist and streams files

Can we change the API to be more like ConfigParser, using get for strings and getint, getfloat and getbool for those respective types? Adapting your example from above:

# get check if global stats is on
nl = Namelist(nlistpath)
dt = nl.getfloat('config_dt')
timeInteg = nl.get('config_time_integrator')
numHalos = nl.getint('config_num_halos')
explicitProcDecomp = nl.getbool('config_explicit_proc_decomp')

# get name for mesh file
sf = XMLList(streamspath)
meshname = sf.read('mesh', 'filename_template')

pwolfram · 2016-10-17T15:03:51Z

@xylar, would we want to have a dictionary-like capability as well as the explicit calling functions? I don't see why not but it may be more elegant (and risky, however) to try to do automatic type-casting with output / access via a dictionary-like structure.

xylar · 2016-10-21T10:44:44Z

@pwolfram, I assume you're still working on updating this PR. Let me know if you're waiting on anything from me.

pwolfram · 2016-10-21T14:19:30Z

@xylar, you are correct-- I have not done anything on this since we chatted Monday. If I'm holding someone up I can increase priority on this and finish it ASAP.

milenaveneziani · 2016-10-21T20:08:56Z

@pwolfram, @xylar: to put it into perspective, this PR, together with a future one on mpas_xarray, have higher priority with respect to anything else, because anything that went into alpha8 and that will go in alpha9 breaks the scripts. In alpha8, we have changed filenames. In alpha9, we will be changing timeSeriesStats instances, and the variable names will change as a consequence (this of course involves changes in this PR and in mpas_xarray/other python scripts). I think it would be good if we could solve these issues in the next couple of weeks, if possible.
Do you think it is feasible?

pwolfram · 2016-10-23T03:10:46Z

@xylar, I've updated the code to reflect our conversation earlier this week. Please let me know what you think. We should have read functionality for namelists and streamfiles now and have testing via pytests, which gets us one step away from CI testing for all PRs in the future.

pwolfram · 2016-10-23T03:11:15Z

P.S. obviously commits need squashed but this can be done after you take a look and before the merge. cc @milenaveneziani

xylar · 2016-10-23T20:33:01Z

mpas_analysis/test/test_namelist_streams_interface.py

+                         '0100_00:00:00')
+
+
+# NOTE, MAY NEED TO SANITIZE NAMELIST AND STREAMS FILES A LITTLE BIT FOR


@pwolfram, I think the example namelists and streams are okay. You can simplify them if you want. But I'd remove this comment either way.

xylar · 2016-10-23T20:44:22Z

@pwolfram, this looks good. I confirm that the tests seem to cover our bases and that they pass with the example namelist and streams file. If you would squash the commits and remove the note i mentioned above (after simplifying the streams file if you like), I will merge.

I don't think there is a particular need for better type checking in this PR. It seems sufficient to me if type errors are raised when the various get* methods of NameList are used incorrectly. If you feel that better type checking is urgently needed, please make these modifications.

milenaveneziani · 2016-10-24T07:33:28Z

@pwolfram, @xylar: thanks a bunch for working on this!
I am eager to try this out on ACME output.

pwolfram · 2016-10-24T21:02:16Z

@xylar, I think this should be ready to merge following your quick double-check on the changes.

xylar · 2016-10-24T21:07:03Z

Great, I'll take a look as soon as I can.

pwolfram · 2016-10-24T21:24:59Z

Thanks @xylar!

xylar · 2016-10-25T09:44:54Z

@pwolfram, I am going to merge this soon. In the future, could you make the description of the PR something that is appropriate as a commit message for the merge? This means it should not include references to other PRs by number and should describe what is in the PR, as opposed to what might be added to the PR. I have modified the commit message to remove/clean up these issues.

xylar · 2016-10-25T09:51:08Z

@pwolfram, I made sure the merged branch passed the tests. The new code doesn't touch the existing analysis in any way so I didn't bother to test that the analysis itself still runs correctly.

xylar · 2016-10-25T09:51:26Z

@pwolfram, please delete the remote branch, since I don't have permission.

pwolfram · 2016-10-25T11:43:11Z

@xylar, thanks for the feedback on the PR description. I'll put checklists with an introduction of "Features of this merge include" and reference other issues in a comment outside the PR description.

pwolfram added enhancement help wanted labels Oct 6, 2016

pwolfram assigned xylar, pwolfram and milenaveneziani Oct 6, 2016

pwolfram mentioned this pull request Oct 6, 2016

Supports ingesting arbitrary MPAS output files (in general, input info from namelist and streams files) #20

Closed

pwolfram mentioned this pull request Oct 7, 2016

Update submodule in PreAndPostProcessingScripts following support for reading namelist and streams files #28

Closed

4 tasks

milenaveneziani reviewed Oct 12, 2016

View reviewed changes

mpas_analysis/shared/io/namelist_streams_interface.py

@@ -0,0 +1,91 @@

#!/usr/bin/python

Copy link

Collaborator

milenaveneziani Oct 12, 2016

Choose a reason for hiding this comment

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

Should be /usr/bin/env python

milenaveneziani reviewed Oct 12, 2016

View reviewed changes

mpas_analysis/test/test_namelist_streams_interface.py

10/07/2016

"""

import os

Copy link

Collaborator

milenaveneziani Oct 12, 2016

Choose a reason for hiding this comment

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

Is this needed?

milenaveneziani reviewed Oct 12, 2016

View reviewed changes

mpas_analysis/test/test_namelist_streams_interface.py

"""

import os

import pytest

Copy link

Collaborator

milenaveneziani Oct 12, 2016

Choose a reason for hiding this comment

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

Is this needed?

milenaveneziani reviewed Oct 12, 2016

View reviewed changes

xylar reviewed Oct 15, 2016

View reviewed changes

xylar reviewed Oct 23, 2016

View reviewed changes

pwolfram force-pushed the namelist_streams_interface branch from 05546c5 to d761229 Compare October 24, 2016 20:58

pwolfram added 2 commits October 24, 2016 17:00

Adds namelist and streams file interfaces

6f460ae

Adds unit tests for namelist / streams reader

b06dd4f

pwolfram force-pushed the namelist_streams_interface branch from d761229 to b06dd4f Compare October 24, 2016 21:00

vanroekel mentioned this pull request Oct 24, 2016

Adds MLD analysis and reduces code in SST analysis #29

Merged

xylar merged commit b06dd4f into MPAS-Dev:master Oct 25, 2016

pwolfram deleted the namelist_streams_interface branch October 25, 2016 11:40

pwolfram mentioned this pull request Oct 25, 2016

Summary of key analysis tasks #32

Closed

22 tasks

		'0100_00:00:00')


		# NOTE, MAY NEED TO SANITIZE NAMELIST AND STREAMS FILES A LITTLE BIT FOR

Adds namelist and streams file interface #27

Adds namelist and streams file interface #27

Uh oh!

Conversation

pwolfram commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwolfram commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

milenaveneziani commented Oct 6, 2016

Uh oh!

pwolfram commented Oct 6, 2016

Uh oh!

pwolfram commented Oct 11, 2016

Uh oh!

pwolfram commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwolfram commented Oct 17, 2016

Uh oh!

xylar commented Oct 21, 2016

Uh oh!

pwolfram commented Oct 21, 2016

Uh oh!

milenaveneziani commented Oct 21, 2016

Uh oh!

pwolfram commented Oct 23, 2016

Uh oh!

pwolfram commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xylar commented Oct 23, 2016

Uh oh!

milenaveneziani commented Oct 24, 2016

Uh oh!

pwolfram commented Oct 24, 2016

Uh oh!

xylar commented Oct 24, 2016

Uh oh!

pwolfram commented Oct 24, 2016

Uh oh!

xylar commented Oct 25, 2016

Uh oh!

xylar commented Oct 25, 2016

Uh oh!

xylar commented Oct 25, 2016

Uh oh!

pwolfram commented Oct 25, 2016

Uh oh!

Uh oh!

pwolfram commented Oct 6, 2016 •

edited

Loading

pwolfram commented Oct 6, 2016 •

edited

Loading

xylar commented Oct 15, 2016 •

edited

Loading