-
Notifications
You must be signed in to change notification settings - Fork 230
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
Uncertainty fixes and user experience improvement #1593
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks great, I added some minor comments
rmgpy/tools/uncertainty.py
Outdated
try: | ||
os.makedirs(folder) | ||
except: | ||
raise Exception('Uncertainty output directory could not be created.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a more specific exception class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I forget why I decided to catch this exception at all. I might have just copied it from somewhere.
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rmgpy/tools/muq.py
Outdated
@@ -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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: PEP 8 style asks to add spaces around operators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
rmgpy/tools/muq.py
Outdated
@@ -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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add %
symbols to the print statement accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put (%)
in the column headers, but I've moved them to the values to make it clearer.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If localAnalysis was set to False but globalAnalysis to True, would we like to print that we're setting localAnalysis to True?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a logging statement.
""" | ||
Assign uncertainties based on the sources of the species thermo and reaction kinetics. | ||
""" | ||
|
||
if gParamEngine is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way to go, auto-formatting tool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, this was manually changed.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type was index
before this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a string, parsed from the csv.
@amarkpayne Would you still like to review this? @alongd Could you take a look at the fixups and documentation additions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great! I just had one minor suggestion for the documentation. Please go ahead and squash. I'll try it out before merging.
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define PCE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I rebased again. Please go ahead and merge whenever you're ready. |
Thanks for this awesome PR! |
Motivation or Problem
This PR contains 3 parts:
Description of Changes
User experience improvements:
simulate.py
script can be similarly used to run uncertainty analysis automaticallyIPython notebook updates:
There are also a number of bug fixes. Documentation updates to be included soon.
Testing
The input files in
ipython/data/ethane_model
andipython/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:
/home/user/anaconda2/envs/rmg_new/lib
to pathconda install hdf5=1.8
conda install icu=54
Reviewer Tips
I suggest going through the commits in order rather than viewing all changes at once.