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

Simplify BurnerFlame simulations #1555

Merged
merged 9 commits into from
Aug 13, 2023
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jul 24, 2023

Changes proposed in this pull request

Unstrained flames have historically been implemented as axisymmetric flames where 'lambda' was used to specify the mass flow rate implicitly (lambda was propagated left-to-right and velocity propagated right-to-left), which leads to confusing boundary conditions (see #1533). The calculation also involves 'V' (which is zero) for the calculation of continuity, which is certainly not intuitive. Beyond, 'V' was also integrated for free flames, although it is by definition zero.

This PR removes the dependency on 'lambda' and 'V', and instead propagates information on mass flow rate left-to-right directly for the 'U' solution component, meaning that 'lambda'/'V' entries are no longer used for unconstrained flames (i.e. BurnerFlame and FreeFlame variants).

In addition:

  • simplify logic used for the evaluation of boundary conditions
  • only evaluate 'V' for strained flames
  • deprecate 'left' outlets, as they are untested and unlikely to work, i.e. only support 'right-to-left' flow for strained flames

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

Closes #1533

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

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #1555 (0260cab) into main (a900edf) will increase coverage by 0.06%.
Report is 37 commits behind head on main.
The diff coverage is 77.63%.

@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
+ Coverage   70.54%   70.61%   +0.06%     
==========================================
  Files         379      379              
  Lines       59110    59135      +25     
  Branches    21232    21252      +20     
==========================================
+ Hits        41698    41757      +59     
+ Misses      14338    14305      -33     
+ Partials     3074     3073       -1     
Files Changed Coverage Δ
src/oneD/Sim1D.cpp 69.20% <ø> (-1.23%) ⬇️
src/oneD/Boundary1D.cpp 62.05% <58.97%> (+6.66%) ⬆️
src/oneD/StFlow.cpp 81.88% <96.77%> (-0.30%) ⬇️
include/cantera/oneD/Boundary1D.h 57.89% <100.00%> (+8.91%) ⬆️
include/cantera/oneD/StFlow.h 100.00% <100.00%> (ø)
src/oneD/refine.cpp 82.26% <100.00%> (+1.41%) ⬆️

... and 24 files with indirect coverage changes

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

@ischoegl ischoegl force-pushed the update-oneD-boundaries branch 2 times, most recently from 2d21280 to 6cb25e2 Compare July 24, 2023 19:31
@ischoegl ischoegl marked this pull request as ready for review July 24, 2023 19:50
@ischoegl ischoegl requested a review from a team July 24, 2023 19:50
@ischoegl
Copy link
Member Author

ischoegl commented Aug 6, 2023

Rebased after merge conflict after #1568; i.e. this is ready for a 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 for looking into this, @ischoegl. I think this generally makes sense, with a few possible adjustments:

  • I like the change to how the energy equation BCs are handled, with the Boundary1D object always responsible for subtracting a term, whether that's the boundary temperature or the value from the fixed temperature profile.

  • I agree that handling right-to-left flow for the unstrained configurations isn't really necessary. Further, there seem to be a number of errors in the BCs in such a case; I tried creating a version of FreeFlame that went in the other direction and couldn't get it to converge in even the simplest of cases. So, if this is non-functional, I think we could just disable it now, without going through the deprecation cycle.

  • Reducing the equations for $\Lambda$ and $V$ to just $\Lambda_j = 0$ and $V_j = 0$ in the free flame and burner flame cases makes sense, but I think it needs to be documented a bit better that this is what's happening.

src/oneD/Boundary1D.cpp Outdated Show resolved Hide resolved
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 Show resolved Hide resolved
src/oneD/StFlow.cpp Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

Thanks for the suggestions, @speth! I addressed all comments, and removed support for left outlets. I suspect that there may be some other issues for right-to-left stagnation flow, but didn't test as this goes beyond the original intent of this PR (which was to untangle boundary conditions for unstrained flow).

One upside is that with this change, it becomes possible - for unstrained cases that do not involve electric fields - to use up to three solution array entries to implement custom equations in overloaded StFlow classes. For example in #1510, a c_offset_Uo could use an existing unused component (although I'm not advocating for this). After 3.0 is out, it would also be possible to not allocate memory for unused components altogether, although this would require significant changes.

@ischoegl ischoegl requested a review from speth August 13, 2023 13:55
@speth
Copy link
Member

speth commented Aug 13, 2023

After 3.0 is out, it would also be possible to not allocate memory for unused components altogether, although this would require significant changes.

Yeah, it would be nice to be more flexible about the state vector components, and have that be determined based on the configuration / domain types. One reason I've hesitated about doing this is that there may be a performance hit in having c_offset_Y and company not being compile-time constants. These variables get used a lot in the evaluation of the governing equations, so I think it's possible this matters, but it also may not.

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, @ischoegl. This looks good to me.

@speth speth merged commit 52309ee into Cantera:main Aug 13, 2023
42 checks passed
@ischoegl ischoegl deleted the update-oneD-boundaries branch August 13, 2023 20:40
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.

Outlet1D conditions
2 participants