-
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
Allow mole fractions to be normalized if they do not sum to one #1809
Conversation
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 think this might not work with mole fraction ranges. Also, liquidReactor
will need to be updated similarly.
rmgpy/rmg/input.py
Outdated
@@ -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()) |
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.
This should be snake_case
. Also, this doesn't seem to account for mole fraction ranges.
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
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 However, if I use a mole fraction range, such as |
@kspieks, what's the status of this PR? |
@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:
|
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. |
No worries at all! It didn't take long to test the liquid reactor |
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 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.
rmgpy/rmg/input.py
Outdated
@@ -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)) | |||
|
|||
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.
Could you remove this spacing change?
rmgpy/rmg/input.py
Outdated
@@ -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 |
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 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
.
logging.info('Normalized mole fractions:') | ||
for spec, molfrac in initialMoleFractions.items(): | ||
logging.info('{0} = {1}'.format(spec, molfrac)) | ||
logging.info('') |
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.
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.
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? |
rmgpy/rmg/input.py
Outdated
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()]): |
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.
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
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.
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!
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
a6030aa
to
629ea74
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
c9d1652
to
c3d6d3b
Compare
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! |
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:
and
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 generalfor key, value in initialMoleFractions.item():
. I assume these should be consistent. What do you recommend? Thanks!