-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Make 1D flow domains agnostic of transport model #1514
Conversation
a6f4f4d
to
fa2895f
Compare
Codecov Report
@@ Coverage Diff @@
## main #1514 +/- ##
==========================================
- Coverage 70.46% 70.42% -0.05%
==========================================
Files 375 375
Lines 58405 58437 +32
Branches 20899 20917 +18
==========================================
- Hits 41158 41152 -6
- Misses 14227 14262 +35
- Partials 3020 3023 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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, I think this makes sense and simplifies flame instantiation a bit. I had just a few suggested changes.
if (solution->transport()->transportModel() == "ionized-gas") { | ||
ret = new IonFlow(solution, id); | ||
} else { | ||
ret = new StFlow(solution, id); | ||
} |
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.
This reliance on the specific model name is a little fragile/limiting, in that it doesn't provide a way for any other transport model to be used with IonFlow
. I believe the only requirement the IonFlow
class puts on the transport model is that it must implement the getMobilities
method. Perhaps we can do something like adding a boolean providesGetMobilities()
method to Transport
that can be reimplemented by any relevant transport model.
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.
Valid comment. However, MixTransport
defines the function also, which complicates this a bit. As long as "ionized-gas" is the only model that implements this, I believe it's safe as long as it's tested. I added two specific tests for all flame types to test_oneD.cpp
, which will fail if any of this is touched.
fa2895f
to
d2ee7e5
Compare
d2ee7e5
to
2a9bb1e
Compare
@speth ... thanks for catching the Regarding your point about the brittle implementation of the |
Changes proposed in this pull request
Switch between
StFlow
andIonFlow
in C++ factory constructors. This allows for the following:IonFlow
behavior in conventionalFreeFlow
/UnstrainedFlow
/AxisymmetricFlow
IonFreeFlame
inFreeFlame
IonBurnerFlame
inBurnerFlame
If applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#165
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage