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

Non-ideal capabilities for 1-D flame model #1079

Merged
merged 19 commits into from Jun 22, 2023
Merged

Conversation

gkogekar
Copy link
Member

@gkogekar gkogekar commented Jul 29, 2021

Changes proposed in this pull request

Add non-ideal capabilities to the existing Flame models. Currently, the flame models utilize only ideal gas phase. There has been some discussion in the user group and the feature request (Cantera/enhancements#109) to include 1-D supercritical features in the Flame code. This pull request implements the real-gas flame models (R-K EoS, P-R EoS) along with the modified transport.

  • Modify StFlow class to include real-gas thermophases (Redlich-Kwong, Peng-Robinson EoS)
  • Modifications to the Transport / HighP transport models (if required)
  • Modifications to Python interface
  • Cantera tests to check real-gas flame capabilities

References
Cantera/enhancements#109

Theory Document
Non_ideal_Flame_equations.pdf

@gkogekar gkogekar closed this May 23, 2022
@gkogekar gkogekar deleted the nonIdealFlame branch May 23, 2022 22:04
@gkogekar gkogekar restored the nonIdealFlame branch May 23, 2022 22:34
@gkogekar gkogekar reopened this May 23, 2022
@speth
Copy link
Member

speth commented May 23, 2022

@gkogekar - If you can, please undo the merge commit and rebase this onto the current main branch instead.

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #1079 (a34b39c) into main (e458764) will increase coverage by 0.02%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
+ Coverage   70.44%   70.46%   +0.02%     
==========================================
  Files         375      375              
  Lines       58401    58405       +4     
  Branches    20897    20899       +2     
==========================================
+ Hits        41142    41158      +16     
+ Misses      14238    14227      -11     
+ Partials     3021     3020       -1     
Impacted Files Coverage Δ
src/oneD/StFlow.cpp 84.16% <94.44%> (+0.23%) ⬆️
include/cantera/oneD/StFlow.h 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gkogekar gkogekar marked this pull request as ready for review June 15, 2022 17:13
@bryanwweber
Copy link
Member

Thanks @gkogekar, I just saw this was ready for review. It looks like there aren't any cases tested where you might expect to see some effect of the non-ideal EOS, can you please add something?

Similarly, the difference with the existing results is somewhat concerning to me. Things are changing in the third and fourth decimal place, (although I don't have a good sense for how much change would be expected). I guess I'm surprised there's so much difference by swapping the enthalpy for the temperature. Can you comment? Can you do a comparison of a simple calculation in the zero-D case to get a sense for how much difference to expect?

@gkogekar
Copy link
Member Author

Thanks, @bryanwweber. I had derived the energy equation for non-ideal EoS (the document is attached above). But we decided to move forward with the finite difference approach (I don't exactly remember why. ) In this implementation, the enthalpy flux is calculated based on a first order finite differencing, as opposed to exact calculation of Cp value in case of ideal gas EoS. So the difference in the results is bound to happen depending on the grid size.

I am creating this PR as many people on the user group have asked for this functionality. But I haven't found any particular test case that will show the non-ideal effects. Since we are modeling flames (i.e. high temperatures), the gas-phase is always near ideal condition. Nonetheless, I will create a test case using a non-ideal EoS.

@bryanwweber
Copy link
Member

Thanks @gkogekar! I think the finite difference makes sense when we don't know anything about the underlying phase, but I wonder if there's room to allow the evaluation to specialize, analogous to Reactor vs. IdealGasReactor? One way I can think of is to add a flag to the constructor like non_ideal_phase (I don't love that name, but I hope it conveys the idea) to allow the user to specify they want to use the finite difference of the enthalpy, which could default to false so that the ideal gas solution is the default. Another way is to auto-detect the phase type; I'm not sure if it's possible to auto-detect the class type since it's a pointer, but that'd be the best case if possible.

As far as examples, I agree that under high-temperature (combustion) conditions, it's really unlikely to see a difference here. That'd be a reasonable test case anyways, if you compare to the ideal gas result. You can also consider something like high-pressure catalysis against a surface, a liquid flow, or something like that.

@gkogekar gkogekar force-pushed the nonIdealFlame branch 2 times, most recently from d51cd9c to e0c0768 Compare June 22, 2022 17:42
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
@gkogekar
Copy link
Member Author

@bryanwweber Thanks for your insights. Retrieving the thermo type in StFlow class is possible. So I have just modified the energy equation code, where it will use finite difference only for non-ideal phases. It will use the existing energy residual equation in the case of ideal gas EoS. This is the simplest workaround based on your suggestions. But I can always add an external flag if necessary.

I will add the test for non-ideal EoS soon.

@bryanwweber
Copy link
Member

But I can always add an external flag if necessary.

Thanks @gkogekar this is a really nice solution, I didn't think it was possible but I'm glad it is! Definitely not something to do here, but I wonder if it makes sense to eliminate some of the class hierarchy in reactors...

@speth speth self-requested a review July 2, 2022 22:16
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 for introducing this PR, @gkogekar. Overall, I think it shows that there isn't all that much required to handle the non-ideal phases in the 1D flame model. Besides the in-line comments below, there are couple of things needed for this PR:

  • Please eliminate the change to the googletest submodule
  • I think we need at least a minimal test case for this. I guess that will require adding an input file that has both transport properties and Redlich-Kwong or Peng-Robinson (or critical states) parameters, which I think we currently lack.
  • A Python example would be nice as well. I don't know what conditions would actually show an effect from the nonideality.
  • The "science" section of the Cantera website will need to be updated as well. I think for posterity, it would be nice to have a simpler derivation of the equations somewhere. The attached PDF is useful, but it has a lot of digressions and dead ends that aren't really necessary.

src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
include/cantera/oneD/StFlow.h Outdated Show resolved Hide resolved
include/cantera/oneD/StFlow.h Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member

Hi @gkogekar ... thanks for updating this PR. Unfortunately, it seems like #1452 introduced a couple of minor merge conflicts since the latest rebase. Apart from this, #1448 introduced a couple of changes that should make the Python side quite a bit easier. Would you mind rebasing so this can be pushed over the finishing line? I'd be happy to assist in case you're facing any hurdles.

@gkogekar
Copy link
Member Author

@ischoegl Thanks for letting me know. I will rebase and resolve the merge conflicts.

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.

@gkogekar ... thank you for the rebase, and for taking care of most of the review comments!

Apart from some minor comments for code style guidelines I left below, there currently are some issues with the test suite. As they're consistent across platforms, you can likely reproduce them locally?

*********************************** Testing Summary ************************************

FAILED: One or more tests failed.
Tests passed: 3447
Up-to-date tests skipped: 0
Tests failed: 301
scons: *** [test_results] Explicit exit, status 1

Failed tests:
    - clib.passed ***no results for entire test suite***
    - oneD.passed ***no results for entire test suite***
    - python ***no results for entire test suite***
    - cxx-flamespeed

****************************************************************************************

src/extensions/pythonExtensions.h Outdated Show resolved Hide resolved
include/cantera/oneD/StFlow.h Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
@gkogekar
Copy link
Member Author

Thanks, @ischoegl. I have taken care of most of the failures from the test suite. But one test (Reaction.PythonExtensibleRate) from Kinetics still fails on my Windows machine, and I am not able to figure out the reason for that. But I can see some other tests are now failing on Ubuntu and Mac platforms as well. Is there any way to fix that?

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 for updating this, @gkogekar -- I think it's looking pretty close. Please see below for a few suggestions.

data/h2o2.yaml Outdated Show resolved Hide resolved
data/h2o2.yaml Outdated Show resolved Hide resolved
include/cantera/oneD/StFlow.h Outdated Show resolved Hide resolved
include/cantera/oneD/StFlow.h Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
test/python/test_onedim.py Show resolved Hide resolved
ext/googletest Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This change to the googletest submodule needs to be eliminated from the commit history. If you need help doing that, let me know and I can force-push an update to this branch that takes care of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea how to do that. I tried to search for the related commit, but failed to find it. Can you help me with that?

Copy link
Member

Choose a reason for hiding this comment

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

I just force-pushed the branch with this change here. Please fetch and reset your local branch to match this one (head is commit d655febfe).

Copy link
Member Author

Choose a reason for hiding this comment

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

@speth Where is this commit d655febfe? The last commit I see is d655feb at https://github.com/gkogekar/cantera/tree/nonIdealFlame

Copy link
Member

Choose a reason for hiding this comment

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

That's the same commit, just truncating the hash at a different point (the full hash is d655febfeebff0c01e6b44effe4b5d64ce4dacba)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I just pushed those changes.

Comment on lines 1319 to 1321
gas = ct.Solution("h2o2.yaml", "ohmech-RK")
gas.TPX = T_in, 0.05 * ct.one_atm, comp
width = 0.2 # m
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be useful to have this regression test at a pressure high enough where the non-ideal model has some effect (if that's even possible for a H2 flame -- I'm not entirely sure).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have modified the test to run at 10 bar, but truly speaking the temperatures are very high. So there is no difference between the idea gas and real gas models.

Copy link
Member

@ischoegl ischoegl Jun 20, 2023

Choose a reason for hiding this comment

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

You could go farther away from stoichiometry? At the same time, this is h2o2.yaml

@speth
Copy link
Member

speth commented Jun 22, 2023

Please don't introduce the merge commit by using git merge or git pull -- this just brings back in the problematic changes.

With your branch checked out, and with all uncommitted changes stashed or committed (or else they will be lost), you should:

  • Run git log and note the commit hashes of your commits after d655feb (e.g. cdf9e8c)
  • Run git reset --hard d655feb
  • Run git cherry-pick cdf9e8c and repeat for any other commits you want to keep.

@gkogekar
Copy link
Member Author

Please don't introduce the merge commit by using git merge or git pull -- this just brings back in the problematic changes.

With your branch checked out, and with all uncommitted changes stashed or committed (or else they will be lost), you should:

  • Run git log and note the commit hashes of your commits after d655feb (e.g. cdf9e8c)
  • Run git reset --hard d655feb
  • Run git cherry-pick cdf9e8c and repeat for any other commits you want to keep.

Apologies for that. I am still not getting used to Git advanced commands. Anyway, I have corrected it and also rebased the code with the latest Cantera build.

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, @gkogekar, this looks good to me. With this completed, could you also open a PR for the cantera-website repository to document the updated 1D flame governing equations?

@speth speth merged commit d717e8b into Cantera:main Jun 22, 2023
40 checks passed
@gkogekar
Copy link
Member Author

Thanks, @gkogekar, this looks good to me. With this completed, could you also open a PR for the cantera-website repository to document the updated 1D flame governing equations?

Thanks, @speth
Sure. I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants