Skip to content
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

Add dilution options to equivalence ratio functions #1206

Merged
merged 5 commits into from
Mar 1, 2022

Conversation

g3bk47
Copy link
Contributor

@g3bk47 g3bk47 commented Feb 22, 2022

Two changes are proposed in this PR:

  1. add a new argument to equivalence_ratio to allow users to ignore certain species, e.g. as a result of dilution with an inert species
  2. add two new arguments to set_equivalence_ratio to dilute a mixture after the mixture composition based on equivalence ratio has been determined.

All changes are backward compatible. Discussion on these changes can be found here:
Cantera/enhancements#108
#521
https://www.google.com/search?q=cantera+users+group&rlz=1C1GCEB_enDE918DE918&oq=canter&aqs=chrome.0.69i59l3j69i57j69i60l3j69i65.6761j0j7&sourceid=chrome&ie=UTF-8

The first change is used like this:
gas.equivalence_ratio(fuel="H2", oxidizer="O2:0.21,N2:0.79", include_species=["H2","O2"])
This computes the equivalence ratio as before, but ignores all species except H2 and O2

The second change is used like this:
gas.set_equivalence_ratio(2.0, "H2", "O2:0.21,N2:0.79", fraction="diluant:0.1", diluant="H2O")
This first creates a mixture of H2 and air with equivalence ratio 2 and then dilutes it with H2O so that the mole fraction of the diluant in the final mixture is 0.1. Similar options exist to fix the fuel and oxygen fraction in the final mixture.

Comprehensive descriptions have been added to the examples in interfaces/cython/cantera/examples/thermo/equivalenceRatio.py to showcase these additions to Cantera.

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Hi @g3bk47 ... thank you for the PR! I believe this is a nice addition that addresses Cantera/enhancements#108 as well as some recent discussions on the user forum.

While there may be another round of suggestions, I mainly focused on the code for the time being. Cantera has a couple of built-in capabilities that are not obvious, but should help simplify some of what you implemented. Also, any new code addition should follow some style conventions more closely: e.g. empty spaces left and right of comparisons and most operators, double quotes for Python strings (whenever possible), etc.

I appreciate that you already added comprehensive unit tests, and updated the example. I honestly haven't checked the actual implementation yet, but it looks like you have most of the bases covered ...

interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_thermo.py Show resolved Hide resolved
interfaces/cython/cantera/test/test_thermo.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_thermo.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
@g3bk47 g3bk47 requested a review from ischoegl February 26, 2022 01:59
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

@g3bk47 ... thank you for the fast turn-around! While I believe that the implementation is sound, I have some additional requests (which are mostly formatting).

interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
@g3bk47 g3bk47 requested a review from ischoegl February 27, 2022 04:28
@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #1206 (da8e2d0) into main (041559d) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1206   +/-   ##
=======================================
  Coverage   65.41%   65.41%           
=======================================
  Files         318      318           
  Lines       46085    46085           
  Branches    19604    19604           
=======================================
+ Hits        30145    30147    +2     
+ Misses      13426    13424    -2     
  Partials     2514     2514           
Impacted Files Coverage Δ
src/base/AnyMap.cpp 85.29% <0.00%> (+0.18%) ⬆️

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 041559d...da8e2d0. Read the comment docs.

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

@g3bk47 … thank you for taking care of all the requested changes! I’ll approve, but leave it open for another day before merging in case anyone else wanted to comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants