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

Fix issue #1593 in evaluating of lambda at boundary point. #1594

Merged
merged 1 commit into from Aug 21, 2023

Conversation

yeanment
Copy link
Contributor

Changes proposed in this pull request

  • right boundary condition for lambda in evalRightBoundary function

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

Closes #1593

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

Fix the bug of non-uniform lambda for strained flow.

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

@yeanment yeanment marked this pull request as ready for review August 18, 2023 04:47
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.

@yeanment ... thanks for the fix! I had inadvertently added the zero gradient to the wrong branch in #1555. My suggestion won't change results, but shows the BC a little better.

src/oneD/StFlow.cpp Show resolved Hide resolved
src/oneD/StFlow.cpp Show resolved Hide resolved
src/oneD/StFlow.cpp Show resolved Hide resolved
@yeanment
Copy link
Contributor Author

I agree with you. This has minor influence on the solution.

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1594 (3d6d17a) into main (cde1e79) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
- Coverage   70.61%   70.60%   -0.01%     
==========================================
  Files         379      379              
  Lines       59153    59152       -1     
  Branches    21252    21252              
==========================================
- Hits        41768    41767       -1     
  Misses      14311    14311              
  Partials     3074     3074              
Files Changed Coverage Δ
src/oneD/StFlow.cpp 81.86% <100.00%> (-0.03%) ⬇️

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

@ischoegl
Copy link
Member

ischoegl commented Aug 19, 2023

@yeanment ... to ensure that the test suite covers this issue, I have suggestions for minor unit test updates, see commit 7b2b855 on https://github.com/ischoegl/cantera/tree/fix-1593 ... feel free to cherry-pick or copy. Also, you can add yourself to the AUTHORS list in the main folder (it's alphabetical).

This fix probably should go in before the release of Cantera 3.0 (which is imminent). Thanks again for catching this issue as well as for creating this PR!

@yeanment
Copy link
Contributor Author

@ischoegl Thank you for your prompt reply. I have checked the commit 7b2b855, believe the unit test updates help cover the issue more comprehensively.

Btw, I am not that familiar with the github workflow. If applicable, you may add me as Shumeng Xie from National University of Singapore. I'm glad I could contribute to this fascinating tool. Thanks again for your effort in maintenance and improvement on this code.

@ischoegl
Copy link
Member

ischoegl commented Aug 21, 2023

Btw, I am not that familiar with the github workflow. If applicable, you may add me as Shumeng Xie from National University of Singapore. I'm glad I could contribute to this fascinating tool. Thanks again for your effort in maintenance and improvement on this code.

No problem! Thanks again for identifying/fixing this issue.

Unfortunately, I'm unable to add to this PR as your yeanment/main branch appears to be protected. The easiest solution is to just accept this PR as is; I will add other changes separately. As a recommendation, it's best to create a new branch on your repository before creating a PR; that way, you won't run into merge conflicts etc.

@ischoegl ischoegl merged commit ec48ebc into Cantera:main Aug 21, 2023
42 checks passed
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.

Non-uniform lambda in using CounterflowTwinPremixedFlame
2 participants