Skip to content

Two temperature plasma reaction#1166

Merged
speth merged 3 commits intoCantera:mainfrom
BangShiuh:ex_reaction
Jan 28, 2022
Merged

Two temperature plasma reaction#1166
speth merged 3 commits intoCantera:mainfrom
BangShiuh:ex_reaction

Conversation

@BangShiuh
Copy link
Copy Markdown
Contributor

@BangShiuh BangShiuh commented Jan 14, 2022 •
edited
Loading

Changes proposed in this pull request

  • Change TwoTempPlasmaReaction to irreversible
  • Enable a short format of input (only A and b) which is very common for this type of reaction.

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

Closes #

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

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

@BangShiuh BangShiuh changed the title Ex reaction Two temperature plasma reaction Jan 14, 2022
@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Jan 14, 2022

@BangShiuh ... sorry for being too eager merging #1099! I had assumed that the PR was ready as all review comments had been addressed 😂

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 15, 2022 •
edited
Loading

Codecov Report

Merging #1166 (865ba76) into main (80e9362) will increase coverage by 0.00%.
The diff coverage is 70.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1166   +/-   ##
=======================================
  Coverage   65.37%   65.37%           
=======================================
  Files         318      318           
  Lines       46137    46145    +8     
  Branches    19612    19615    +3     
=======================================
+ Hits        30160    30166    +6     
- Misses      13474    13475    +1     
- Partials     2503     2504    +1
Impacted Files Coverage Δ
include/cantera/kinetics/Arrhenius.h 100.00% <ø> (ø)
include/cantera/kinetics/Reaction.h 100.00% <ø> (ø)
src/kinetics/Reaction.cpp 79.22% <60.00%> (-0.09%) ⬇️
src/kinetics/Arrhenius.cpp 90.58% <80.00%> (+0.16%) ⬆️

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 80e9362...865ba76. Read the comment docs.

@BangShiuh
Copy link
Copy Markdown
Contributor Author

BangShiuh commented Jan 17, 2022

@ischoegl I keep getting NAN value for my reaction rate even if I force m_Ea_R and m_EE_R to be zero.

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Jan 19, 2022

Hi @BangShiuh ... could you rebase on main? Some of the test failures reference tests that aren't part of your commit history yet (I believe GH does an automatic rebase on HEAD whenever CI is run).

speth
speth requested changes Jan 24, 2022
View reviewed changes
Copy link
Copy Markdown
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.

In addition to rebasing, here are a couple of other suggestions -- I think the NAN-equality issue I pointed out might be the cause of your test failures.

@BangShiuh
Copy link
Copy Markdown
Contributor Author

BangShiuh commented Jan 26, 2022

@speth I have fixed those issues. Good to go?

ischoegl
ischoegl reviewed Jan 26, 2022
View reviewed changes
Copy link
Copy Markdown
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.

No comments from my side, as all my concerns had already been addressed in a previous PR.

speth
speth requested changes Jan 27, 2022
View reviewed changes
Copy link
Copy Markdown
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 for the updates in response to the previous review, and I'm glad to see all the tests are now passing.

The only thing left is that there are a couple of checks for NAN that I think are unnecessary.

@speth speth merged commit 9cee052 into Cantera:main Jan 28, 2022
@BangShiuh BangShiuh deleted the ex_reaction branch April 29, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ischoegl ischoegl ischoegl left review comments

@speth speth speth approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BangShiuh @ischoegl @speth