-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Conversation
Codecov Report
@@ 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
... and 24 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2d21280
to
6cb25e2
Compare
6cb25e2
to
057bb46
Compare
Rebased after merge conflict after #1568; i.e. this is ready for a review. |
There was a problem hiding this 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.
Remove support for unstrained right-to-left flow, i.e. restrict right-to-left flow to strained configurations.
1eca5a2
to
6c7c5cb
Compare
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 |
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 |
There was a problem hiding this 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.
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
andFreeFlame
variants).In addition:
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
scons build
&scons test
) and unit tests address code coverage