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

YAML surface phase requires kinetics model #1247

Merged
merged 4 commits into from Apr 20, 2022
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Apr 19, 2022

Changes proposed in this pull request

  • Make default kinetics type dependent on phase dimensions

If applicable, fill in the issue number this pull request is fixing

Closes #1244

If applicable, provide an example illustrating new features this pull request is introducing

updated ...

For the YAML input (say, test.yaml):

phases:
- name: test
  thermo: ideal-surface
  species: [{ptcombust.yaml/species: all}]
  state: {T: 300, P: 1 atm}

Importing in Python yields:

>>> import cantera as ct
>>> p = ct.Interface("test.yaml")
>>> p.reactions()
[]
>>> p.species_names
['PT(S)', 'H(S)', 'H2O(S)', 'OH(S)', 'CO(S)', 'CO2(S)', 'CH3(S)', 'CH2(S)s', 'CH(S)', 'C(S)', 'O(S)']

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #1247 (ef90925) into main (67bac69) will decrease coverage by 0.00%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
- Coverage   65.47%   65.47%   -0.01%     
==========================================
  Files         327      327              
  Lines       46416    46424       +8     
  Branches    19718    19726       +8     
==========================================
+ Hits        30391    30396       +5     
- Misses      13496    13497       +1     
- Partials     2529     2531       +2     
Impacted Files Coverage Δ
src/thermo/ThermoFactory.cpp 71.19% <75.00%> (-0.20%) ⬇️
src/kinetics/KineticsFactory.cpp 90.56% <84.61%> (-1.36%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@speth
Copy link
Member

speth commented Apr 19, 2022

phases:
- name: test
  thermo: ideal-surface
  species: [{ptcombust.yaml/species: all}]
  reactions: none
  state: {T: 300, P: 1 atm}

I think the goal should be to allow the construction of a Solution or InterfacePhase object, and probably an Interface without having to specify anything about the kinetics or reactions. The behavior here is actually a bit surprising -- we currently regard specifying a reactions key without a kinetics key as an error, and I think that behavior should probably be maintained (at least, as long as the idea of having different kinetics models lasts).

@ischoegl
Copy link
Member Author

ischoegl commented Apr 20, 2022

@speth … thanks for your comment!

I actually had originally implemented a version that didn’t require reactions: none, but opted for a simpler version that didn’t need a logic change. Based on your comment, I added things back in, which does change one example in the test suite.

@ischoegl ischoegl marked this pull request as draft April 20, 2022 00:32
@ischoegl ischoegl force-pushed the fix-1244 branch 2 times, most recently from 70df2c6 to e14b4bd Compare April 20, 2022 01:13
@ischoegl ischoegl marked this pull request as ready for review April 20, 2022 02:32
@ischoegl
Copy link
Member Author

@speth ... I believe this is ready for review.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. I think the bit of extra implementation complexity is worth it for the more straightforward end-user experience.

@speth speth merged commit 44bb6ff into Cantera:main Apr 20, 2022
@ischoegl ischoegl deleted the fix-1244 branch April 28, 2022 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YAML surface phase requires kinetics model
2 participants