Allow mole fractions to be normalized if they do not sum to one #1809

kspieks · 2019-11-05T18:50:22Z

Added some code in rmgpy/rmg/input.py that normalizes the mole fractions if necessary. This resolves issue #1750

Motivation or Problem

RMG previously would normalize initial mole fractions specified in input files. This functionality was removed in #1331.

Description of Changes

Added some code in rmgpy/rmg/input.py that normalizes the mole fractions if necessary

Testing

I tested this on the superminimal example with two conditions:

initialMoleFractions={
        'H2': 0.67, 'O2': 0.33,
    },

and

initialMoleFractions={
        'H2': 67, 'O2': 33,
    },

The feature seems to work as expected

Reviewer Tips

Minor question: I used for spec, molfrac in initialMoleFractions.items(): to be explicit about what we were looping over. The code immediately above this addition in line 195 uses a more general for key, value in initialMoleFractions.item():. I assume these should be consistent. What do you recommend? Thanks!

mliu49

I think this might not work with mole fraction ranges. Also, liquidReactor will need to be updated similarly.

mliu49 · 2019-11-06T15:33:01Z

rmgpy/rmg/input.py

@@ -206,6 +206,20 @@ def simple_reactor(temperature,
            elif value[1] < value[0]:
                raise InputError('Initial mole fraction range out of order: {0}'.format(key))

+    # normalize mole fractions if applicable
+    totalInitialMoles = sum(initialMoleFractions.values())


This should be snake_case. Also, this doesn't seem to account for mole fraction ranges.

mliu49 · 2019-11-06T18:40:33Z

It seems that ranged reactors do their own normalization here.

So we can probably just do this for fixed initial mole fractions.

kspieks · 2019-11-07T01:35:02Z

It seems that ranged reactors do their own normalization here.

So we can probably just do this for fixed initial mole fractions.

As a fix, I moved the normalization code to underneath the

if not isinstance(temperature, list) and not isinstance(pressure, list) and all( [not isinstance(x, list) for x in initialMoleFractions.values()]): 
    nSims = 1

for both the simple and liquid reactor. This successfully normalizes when the mole fractions are given as scalars, not lists. Upon testing the superminimal example with both initialMoleFractions={ 'H2': 0.67, 'O2': 0.33, } and initialMoleFractions={ 'H2': 67, 'O2': 33, }, I observe the expected normalization behavior since the final plot is always scaled between 0 and 0.7.

However, if I use a mole fraction range, such as initialMoleFractions={ 'H2': [67, 77], 'O2': 33, }, the final plot is scaled from 0 to 70, while using initialMoleFractions={ 'H2': [0.67, 0.77], 'O2': 0.33, } scales from 0 to 0.7. It seems like the range reactor is not normalizing? I think I'm confused on the behavior of the range reactor

mliu49 · 2019-12-05T15:03:50Z

@kspieks, what's the status of this PR?

kspieks · 2019-12-05T22:24:08Z

@mliu49 I believe this PR is now ready to be merged based on the tests I ran.

I removed the feature from the liquidReactor because running the liquid_phase example revealed significant changes to the behavior of the liquidReactor by causing it to perform many more iterations than running the same liquid_phase example on the master branch commit 9a46923 aka updated this afternoon. I had to kill the example because the plot in the solver directory was different, and it had not converged after twice as many examples so the statistics file was larger and had additional species and reactions in the core and edge. Presumably this is because the liquidReactor is expecting initialConcentrations, thus giving it mole fractions changes the behavior. I propose leaving the liquidReactor as it was on master.

In contrast, simpleReactor is expecting initialMoleFractions so the normalization features works as expected. The superminimal example was tested under 3 cases. The results from running on this branch vs master commit 9a46923 aka updated this afternoon are compared:

initialMoleFractions={ 'H2': 0.67, 'O2': 0.33, }
The number of species and reactions in the core and edge from the statistics files are identical, the final plot in the solver directory looks identical, and diffing the final simulation csv also proves they are identical.
initialMoleFractions={ 'H2': 67, 'O2': 33, }
The number of species and reactions in the core and edge from the statistics files are identical, and the final plot in the solver directory looks identical. The final simulation csv is different due to H2 being 67 in one and 0.67 in another. This also changes the numbers in all columns by a factor of 100. Strangely, the ODE solver seemed to take slightly different steps making it difficult to compare. However, recursively diffing the seed directory proves that the files in the filters, seed, and seed_edge directory are identical.
initialMoleFractions={ 'H2’:[0.50, 0.70], 'O2': 0.33, }
The number of species and reactions in the core and edge from the statistics files are identical, the final plot in the solver directory looks identical, and diffing the final simulation csv also proves they are identical

mliu49 · 2019-12-06T20:55:46Z

You're right, liquid reactor does not need normalization since it uses concentrations. I wasn't thinking properly. Sorry for the extra work you had to do to try that.

kspieks · 2019-12-06T21:00:14Z

No worries at all! It didn't take long to test the liquid reactor

mliu49

I made some minor comments, but I think this is close to being merged. Once you fix the comments, I would wait to rebase until #1842 is merged.

mliu49 · 2019-12-06T20:56:32Z

rmgpy/rmg/input.py

@@ -205,7 +205,7 @@ def simple_reactor(temperature,
                raise InputError('Initial mole fractions cannot be negative.')
            elif value[1] < value[0]:
                raise InputError('Initial mole fraction range out of order: {0}'.format(key))
-
+        


Could you remove this spacing change?

mliu49 · 2019-12-06T21:03:59Z

rmgpy/rmg/input.py

@@ -225,6 +225,19 @@ def simple_reactor(temperature,
    if not isinstance(temperature, list) and not isinstance(pressure, list) and all(
            [not isinstance(x, list) for x in initialMoleFractions.values()]):
        nSims = 1
+        # normalize mole fractions if applicable


I think we may want this even if T and P are ranges, since those are independent parameters.

I would add a second if statement here with only all([not isinstance(x, list) for x in initialMoleFractions.values()]). Also, the comment could be updated to say if not using a mole fraction range.

mliu49 · 2019-12-06T21:09:03Z

rmgpy/rmg/input.py

+            logging.info('Normalized mole fractions:')
+            for spec, molfrac in initialMoleFractions.items():
+                logging.info('{0} = {1}'.format(spec, molfrac))
+            logging.info('')


This is really minor, but I think it would be nice to put the blank lines before Original composition and Normalized mole fractions like before.

kspieks · 2019-12-06T21:30:58Z

Thanks for the feedback! I pushed the changes that addressed your comments (though I still have to look up how to undo whitespace in a pushed commit). Any idea when #1842 will be merged?

mliu49 · 2019-12-06T21:37:57Z

rmgpy/rmg/input.py

-                logging.info('{0} = {1}'.format(spec, molfrac))
-            logging.info('')
+        # normalize mole fractions if not using a mole fraction range
+        if all([not isinstance(x, list) for x in initialMoleFractions.values()]):


This should be indented to the same level as the above if statement:

if T,P,X are not ranges: nSims=1 if X are not ranges: normalize mole fractions

My apologies, I was rushing to get to the mini TG 😬 The changes should be correct now. All commits have been squashed and rebased onto master. Thanks again for catching my mistakes!

mliu49 · 2019-12-06T21:40:36Z

You can add another commit to remove the added whitespace. I would suggest squashing all of these commits together since it's a relatively small change. You can do that when you rebase onto master.

#1842 will be merged once all tests pass, maybe within the next hour or so.

Add code to input.py to normalize mole fractions for simpleReactor if not using a mole fraction range since ranged reactors already normalize. Temp

codecov · 2019-12-06T23:12:23Z

Codecov Report

Merging #1809 into master will increase coverage by 0.23%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1809      +/-   ##
=========================================
+ Coverage   43.96%   44.2%   +0.23%     
=========================================
  Files          83      83              
  Lines       21518   21533      +15     
  Branches     5639    5645       +6     
=========================================
+ Hits         9461    9519      +58     
+ Misses      11010   10947      -63     
- Partials     1047    1067      +20

Impacted Files	Coverage Δ
rmgpy/rmg/input.py	`42.64% <100%> (+8.64%)`	⬆️
arkane/kinetics.py	`12.14% <0%> (ø)`	⬆️
rmgpy/data/statmech.py	`42.2% <0%> (ø)`	⬆️
rmgpy/rmg/pdep.py	`12.21% <0%> (ø)`	⬆️
rmgpy/data/kinetics/family.py	`49.06% <0%> (ø)`	⬆️
rmgpy/statmech/ndTorsions.py	`59.78% <0%> (ø)`	⬆️
rmgpy/yml.py	`15.71% <0%> (ø)`	⬆️
arkane/sensitivity.py	`10% <0%> (ø)`	⬆️
rmgpy/data/kinetics/database.py	`50.61% <0%> (ø)`	⬆️

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 d4f77cd...c3d6d3b. Read the comment docs.

mliu49 · 2019-12-09T16:18:00Z

I think this is ready to be merged. Could someone else take a look at the unit tests that I added? @kspieks @mjohnson541

kspieks · 2019-12-09T17:12:17Z

I think this is ready to be merged. Could someone else take a look at the unit tests that I added? @kspieks @mjohnson541

I thought the unit tests looked good. I should've thought to add those, so thanks for doing that!

kspieks added Complexity: Low Good First Issue labels Nov 5, 2019

kspieks requested a review from mliu49 November 5, 2019 18:50

kspieks self-assigned this Nov 5, 2019

mliu49 reviewed Nov 6, 2019

View reviewed changes

mliu49 added the For Next Release label Nov 19, 2019

mliu49 reviewed Dec 6, 2019

View reviewed changes

Normalize mole fractions for simpleReactor

629ea74

Add code to input.py to normalize mole fractions for simpleReactor if not using a mole fraction range since ranged reactors already normalize. Temp

kspieks force-pushed the normalize_mole_fractions branch from a6030aa to 629ea74 Compare December 6, 2019 23:12

Add unit tests for initial composition handling in reactor input blocks

c3d6d3b

mliu49 force-pushed the normalize_mole_fractions branch from c9d1652 to c3d6d3b Compare December 7, 2019 22:33

mliu49 approved these changes Dec 9, 2019

View reviewed changes

mliu49 merged commit 17bfb68 into master Dec 9, 2019

mliu49 deleted the normalize_mole_fractions branch December 9, 2019 17:21

This was referenced Dec 13, 2019

RMG v3.0.0 Release Planning #1830

Closed

RMG-Py v3.0.0 Release #1852

Merged

hwpang mentioned this pull request Jul 30, 2020

Reactors no longer normalize mole fractions #1750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mole fractions to be normalized if they do not sum to one #1809

Allow mole fractions to be normalized if they do not sum to one #1809

kspieks commented Nov 5, 2019

mliu49 left a comment

mliu49 Nov 6, 2019

mliu49 commented Nov 6, 2019

kspieks commented Nov 7, 2019 •

edited

mliu49 commented Dec 5, 2019

kspieks commented Dec 5, 2019 •

edited by mliu49

mliu49 commented Dec 6, 2019

kspieks commented Dec 6, 2019

mliu49 left a comment

mliu49 Dec 6, 2019

mliu49 Dec 6, 2019

mliu49 Dec 6, 2019

kspieks commented Dec 6, 2019

mliu49 Dec 6, 2019

kspieks Dec 6, 2019

mliu49 commented Dec 6, 2019

codecov bot commented Dec 6, 2019 •

edited

mliu49 commented Dec 9, 2019

kspieks commented Dec 9, 2019

Allow mole fractions to be normalized if they do not sum to one #1809

Allow mole fractions to be normalized if they do not sum to one #1809

Conversation

kspieks commented Nov 5, 2019

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

mliu49 left a comment

Choose a reason for hiding this comment

mliu49 Nov 6, 2019

Choose a reason for hiding this comment

mliu49 commented Nov 6, 2019

kspieks commented Nov 7, 2019 • edited

mliu49 commented Dec 5, 2019

kspieks commented Dec 5, 2019 • edited by mliu49

mliu49 commented Dec 6, 2019

kspieks commented Dec 6, 2019

mliu49 left a comment

Choose a reason for hiding this comment

mliu49 Dec 6, 2019

Choose a reason for hiding this comment

mliu49 Dec 6, 2019

Choose a reason for hiding this comment

mliu49 Dec 6, 2019

Choose a reason for hiding this comment

kspieks commented Dec 6, 2019

mliu49 Dec 6, 2019

Choose a reason for hiding this comment

kspieks Dec 6, 2019

Choose a reason for hiding this comment

mliu49 commented Dec 6, 2019

codecov bot commented Dec 6, 2019 • edited

Codecov Report

mliu49 commented Dec 9, 2019

kspieks commented Dec 9, 2019

kspieks commented Nov 7, 2019 •

edited

kspieks commented Dec 5, 2019 •

edited by mliu49

codecov bot commented Dec 6, 2019 •

edited