Uncertainty fixes and user experience improvement #1593

mliu49 · 2019-05-02T19:40:37Z

Motivation or Problem

This PR contains 3 parts:

Minor maintenance of uncertainty analysis functionality to make sure it works
Major user experience improvements in how uncertainty analysis is performed
Updates to related IPython notebooks and examples

Description of Changes

User experience improvements:

It is now possible to request uncertainty analysis be automatically performed at the end of an RMG job via input file settings
The simulate.py script can be similarly used to run uncertainty analysis automatically
Uncertainty values for uncorrelated global analysis can be retrieved automatically instead of manually entered

IPython notebook updates:

findParameterSourcesAndAssignUncertainties.ipynb
localUncertainty.ipynb
globalUncertainty.ipynb
canteraSimulation.ipynb
canteraSensitivityComparison.ipynb

There are also a number of bug fixes. Documentation updates to be included soon.

Testing

The input files in ipython/data/ethane_model and ipython/data/pdd_model can be run to test uncertainty analysis following an RMG job. All of the IPython notebooks should work. Note that globalUncertainty does require setting up MUQ properly, which is non-trivial currently.

To set up MUQ:

I suggest creating a new environment first
Add /home/user/anaconda2/envs/rmg_new/lib to path
Do conda install hdf5=1.8
Do conda install icu=54

Reviewer Tips

I suggest going through the commits in order rather than viewing all changes at once.

codecov · 2019-05-02T20:39:50Z

Codecov Report

Merging #1593 into master will decrease coverage by 0.09%.
The diff coverage is 20.4%.

@@           Coverage Diff            @@
##           master   #1593     +/-   ##
========================================
- Coverage   41.59%   41.5%   -0.1%     
========================================
  Files         176     176             
  Lines       29150   29306    +156     
  Branches     5992    6033     +41     
========================================
+ Hits        12126   12163     +37     
- Misses      16178   16292    +114     
- Partials      846     851      +5

Impacted Files	Coverage Δ
rmgpy/tools/canteraModel.py	`38.87% <0%> (ø)`	⬆️
rmgpy/tools/muq.py	`8.48% <10.89%> (-0.57%)`	⬇️
rmgpy/rmg/input.py	`35.77% <25%> (-0.08%)`	⬇️
rmgpy/tools/uncertainty.py	`38.46% <27.96%> (-3.82%)`	⬇️
rmgpy/rmg/main.py	`23.07% <5.31%> (+0.34%)`	⬆️
rmgpy/tools/simulate.py	`71.69% <75%> (-5.67%)`	⬇️
rmgpy/tools/plot.py	`45.93% <84.61%> (+1.02%)`	⬆️
rmgpy/data/kinetics/family.py	`52.9% <0%> (+0.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6c78ca...7e15900. Read the comment docs.

alongd

The code looks great, I added some minor comments

alongd · 2019-05-09T19:44:57Z

rmgpy/tools/uncertainty.py

+                try:
+                    os.makedirs(folder)
+                except:
+                    raise Exception('Uncertainty output directory could not be created.')


Can we use a more specific exception class?

Fixed. I forget why I decided to catch this exception at all. I might have just copied it from somewhere.

alongd · 2019-05-09T19:47:28Z

rmgpy/tools/muq.py

@@ -81,6 +81,29 @@ def __init__(self, cantera, outputSpeciesList, kParams, kUncertainty, gParams, g
        # The size of the uncertain inputs: [parameters affecting k, parameters affecting free energy G]         
        self.inputSize = [len(kParams) + len(gParams)]

+        if not self.correlated:


Above, before class ReactorModPiece(ModPiece): there are installation instructions for MUQ. Do these require an update in light of the recommendation you gave in the PR message?

I'm not sure. Ideally, we should update the MUQ binaries so none of this is an issue anymore. Idk when I'll get a chance to look into doing so though.

alongd · 2019-05-09T19:48:30Z

rmgpy/tools/muq.py

@@ -476,8 +476,8 @@ def analyzeResults(self):
                            description = 'dln[{0}]/dln[{1}]'.format(outputSpecies.toChemkin(), descriptor)

                        print '{0:55} {1:10.3f} {2:10.3f}'.format(description,
-                                                                  mainSens[outputIndex][parameterIndex],
-                                                                  totalSens[outputIndex][parameterIndex])
+                                                                  100*mainSens[outputIndex][parameterIndex],


Minor: PEP 8 style asks to add spaces around operators

alongd · 2019-05-09T19:49:18Z

rmgpy/tools/muq.py

@@ -476,8 +476,8 @@ def analyzeResults(self):
                            description = 'dln[{0}]/dln[{1}]'.format(outputSpecies.toChemkin(), descriptor)

                        print '{0:55} {1:10.3f} {2:10.3f}'.format(description,
-                                                                  mainSens[outputIndex][parameterIndex],


Shouldn't we add % symbols to the print statement accordingly?

I had put (%) in the column headers, but I've moved them to the values to make it clearer.

alongd · 2019-05-09T19:51:42Z

rmgpy/rmg/input.py

+def uncertainty(localAnalysis=False, globalAnalysis=False, uncorrelated=True, correlated=True,
+                localNumber=10, globalNumber=5, terminationTime=None, pceRunTime=1800):
+    rmg.uncertainty = {
+        'local': localAnalysis if not globalAnalysis else True,  # Must run local before global


If localAnalysis was set to False but globalAnalysis to True, would we like to print that we're setting localAnalysis to True?

Added a logging statement.

alongd · 2019-05-09T19:54:38Z

rmgpy/tools/uncertainty.py

        """
        Assign uncertainties based on the sources of the species thermo and reaction kinetics.
        """
-
+        if gParamEngine is None:


Way to go, auto-formatting tool!

Haha, this was manually changed.

alongd · 2019-05-09T19:57:01Z

rmgpy/tools/plot.py

@@ -96,12 +96,12 @@ def parseCSVData(csvFile):
            rxn = data.label.split()[1]
            index = data.label.split()[0][:-2].rpartition('dln[k')[2]
            data.reaction = rxn
-            data.index = index
+            data.index = int(index)


What type was index before this change?

It was a string, parsed from the csv.

mliu49 · 2019-06-03T16:23:11Z

@amarkpayne Would you still like to review this?

@alongd Could you take a look at the fixups and documentation additions?

alongd

Code looks great! I just had one minor suggestion for the documentation. Please go ahead and squash. I'll try it out before merging.

alongd · 2019-06-04T13:12:42Z

documentation/source/users/rmg/input.rst

+
+Finally, there are a few miscellaneous options for global uncertainty analysis. The ``terminationTime`` applies for the
+reactor simulation. It is only necessary if termination time is not specified in the reactor settings (i.e. only other
+termination criteria are used). The ``pceRunTime`` sets the time limit for fitting the PCE to the output surface.


Define PCE?

I added the abbreviation to the first mention of Polynomial Chaos Expansion earlier in the section (line 806).

Vlist was not being properly passed as an argument

Create RMG.run_model_analysis method to do sensitivity and uncertainty

Output sensitivity indices as percentages Log results by default, print directly if requested

For when the saturated version of a molecule is not in the model

To avoid some cyclic imports, and it seems like a better location

For the default temperatures in the thermo block, the lower bound should be 10 characters.

And update example data files

If multiple species were included in the local analysis, then a given parameter might appear more than once. However, we only want to include it once for global analysis.

So user can toggle whether to perform the analysis for ln(x) or just x

Remove ipython/uncertainty files since new example files have been added to ipython/data

New ethane model was generated for the uncertainty notebooks, so this updates the Cantera notebooks to use the same files and removes the old mechanisms.

mliu49 · 2019-06-07T16:52:26Z

I rebased again. Please go ahead and merge whenever you're ready.

alongd · 2019-06-07T20:16:15Z

Thanks for this awesome PR!

mliu49 added Status: Ready for Review PR is complete and ready to be reviewed Type: User Experience labels May 2, 2019

mliu49 requested review from mjohnson541, alongd and amarkpayne May 2, 2019 19:40

mliu49 self-assigned this May 2, 2019

alongd reviewed May 9, 2019

View reviewed changes

alongd reviewed Jun 4, 2019

View reviewed changes

mliu49 force-pushed the uncertainty_fixes branch from a8e4f9b to c34b8c6 Compare June 4, 2019 15:08

mliu49 added 19 commits June 7, 2019 12:50

Automatically format rmgpy.tools.uncertainty

1b88161

Automatically format rmgpy.tools.muq

ad168ba

Adjust file paths for local uncertainty analysis

ea79bcd

Automatically retrieve internal indices

e1bbf01

Return uncertainty results

a27e59a

Retrieve uncertainties for uncorrelated global analysis

abc551c

Perform PCE fitting in ln(x) space

cfb7607

Bugfix in rmgpy.tools.canteraModel

d2c213d

Vlist was not being properly passed as an argument

Add reactionSystemIndex argument to Uncertainty.localAnalysis

ebdcabb

Add function to process and prettify local uncertainty results

11c7cdb

Add input block for uncertainty settings

3b39af9

Enable automatic uncertainty analysis following RMG job

6852f51

Create RMG.run_model_analysis method to do sensitivity and uncertainty

Replace labels with species in sensConditions

467f436

Bugfix: Cast parsed indices to int when loading sensitivity results

b7d5fe9

For local uncertainty, save RMG index for reactions

cfd07f0

Refactor output printing for global uncertainty analysis

879d7db

Output sensitivity indices as percentages Log results by default, print directly if requested

Save extra species created for correlated uncertainty analysis

da82473

For when the saturated version of a molecule is not in the model

Split run_uncertainty_analysis off of run_model_analysis

e3cf9ba

Move plot_sensitivity to rmgpy.tools.plot

1870e87

To avoid some cyclic imports, and it seems like a better location

mliu49 added 13 commits June 7, 2019 12:50

Enable simulate.py to run uncertainty analysis

2e55f78

Make file naming in initializeLog more generic

b8662d4

Fix formatting issue in writing Chemkin files

37dbd53

For the default temperatures in the thermo block, the lower bound should be 10 characters.

Update findParameterSourcesAndAssignUncertainties.ipynb

035e481

And update example data files

Bugfix: Disable species constraints for training reactions

8494c36

Check if global analysis parameters are already in list

92bd96b

If multiple species were included in the local analysis, then a given parameter might appear more than once. However, we only want to include it once for global analysis.

Update localUncertainty.ipynb and example models

9502978

Make ln(mole fraction) an option for global uncertainty analysis

75a9ff6

So user can toggle whether to perform the analysis for ln(x) or just x

Update globalUncertainty.ipynb

65f3db0

Remove ipython/uncertainty files since new example files have been added to ipython/data

Update Cantera simulation IPython notebooks and data files

3d36c5c

New ethane model was generated for the uncertainty notebooks, so this updates the Cantera notebooks to use the same files and removes the old mechanisms.

Inform user if local uncertainty is being enabled

3a06b6a

Add documentation on using uncertainty options in input file

a3ab31d

Add note about uncertainty to simulate.py documentation

7e15900

mliu49 force-pushed the uncertainty_fixes branch from c34b8c6 to 7e15900 Compare June 7, 2019 16:51

alongd approved these changes Jun 7, 2019

View reviewed changes

alongd merged commit 85c81ea into master Jun 7, 2019

alongd deleted the uncertainty_fixes branch June 7, 2019 20:16

mliu49 mentioned this pull request Jun 10, 2019

RMG v2.4.0 (?) Release Planning #1591

Closed

5 tasks

mliu49 mentioned this pull request Jul 2, 2019

Delete simulation files after unit testing #1645

Merged

mliu49 mentioned this pull request Jul 25, 2019

Our iPython notebooks must be updated #1533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uncertainty fixes and user experience improvement #1593

Uncertainty fixes and user experience improvement #1593

mliu49 commented May 2, 2019

codecov bot commented May 2, 2019 •

edited

alongd left a comment

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

alongd May 9, 2019

mliu49 May 10, 2019

mliu49 commented Jun 3, 2019

alongd left a comment

alongd Jun 4, 2019

mliu49 Jun 4, 2019

mliu49 commented Jun 7, 2019

alongd commented Jun 7, 2019

Uncertainty fixes and user experience improvement #1593

Uncertainty fixes and user experience improvement #1593

Conversation

mliu49 commented May 2, 2019

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

codecov bot commented May 2, 2019 • edited

Codecov Report

alongd 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

mliu49 commented Jun 3, 2019

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mliu49 commented Jun 7, 2019

alongd commented Jun 7, 2019

codecov bot commented May 2, 2019 •

edited