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
[Transport] Add unity Lewis number transport model #510
Conversation
I am not sure why the OSx test is failing, but it appears unrelated to the current pull request, since the Linux test passed and the code changes should be agnostic to the operating system. @speth you have reviewed and commented in detail the earlier pull request (#427). I believe I have considered most (all) your comments in this one and would appreciate if you could review this. |
The error in the macOS build is because of Homebrew Can you add some tests for this new functionality? Tests for the Python interface go in the |
@bryanwweber: I added a test with some basic sanity checks. The test passes and I think this is good to go on that end. |
I think this implementation of If we start with Eq. 3.203 from Chemically Reacting Flow (Kee, Coltrin, Glarborg; 1st ed.), and neglect the pressure and viscous dissipation terms, we have: If we define jk = - ρ Dkm ∇ Yk, (Eq. 12.177) which corresponds to the diffusion coefficients returned by the On the other hand, if we define jk = - ρ Yk/Xk Dkm' ∇Xk (from Eq. 12.179), the definition which corresponds to I believe the same issue will occur with |
@speth From your analysis, do you think that a consistent expression for the diffusion coefficient under the Le=1 assumption can be derived for each of the 3 cases? |
What I think I have shown is that there is no definition of a single value (for all species) which could be returned by |
@speth Under what conditions would the different calls to |
Well, you could do that, but the 1D flame code in Cantera uses the |
Apologies for the late reply on this issue, I could not find my original notes on the derivation of the unity Lewis number model anymore and then got caught up in other tasks. @speth: In your derivation above, you did not consider the correction velocity which is required to ensure \sum j_k = 0. If you consider the correction velocity as defined in Kee et al. (2003), Eq. 12.183, the extra terms (containing the mean molecular weight) will drop out. Because the correction velocity is also implemented in Cantera I thus believe that my implementation of the unity Lewis number model is correct, and yields the properties you outlined above. I have attached a document in which I derive the respective expressions: Regarding the implementation, as you noted above, Cantera's 1D flame solver uses |
@awehrfritz Thanks, that derivation is very helpful. The fact that applying that form of the correction velocity fixes the issue is not very intuitive (at least to me), but it does indeed work out as you have shown. In the case of |
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.
I think this is just about good to go, pending some minor style cleanup.
* for each species (m^2/s). length m_nsp | ||
*/ | ||
virtual void getMixDiffCoeffs(doublereal* const d){ | ||
// writelog("Unity Lewis number approximation\n"); |
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.
Please delete the commented out writelog
statement.
* @param[out] d Vector of mixture diffusion coefficients, \f$ D_{m} \f$ , | ||
* for each species (m^2/s). length m_nsp | ||
*/ | ||
virtual void getMixDiffCoeffs(doublereal* const d){ |
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.
Please use double
instead of doublereal
(as noted in CONTRIBUTING.md)
mu2 = self.phase.viscosity | ||
lambda2 = self.phase.thermal_conductivity | ||
alpha2 = lambda2/(self.phase.density*self.phase.cp) | ||
Dkm2 = self.phase.mix_diff_coeffs |
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.
Dkm2
is assigned but never used
self.assertNear(mu1, mu2) | ||
self.assertNear(lambda1, lambda2) | ||
self.assertArrayNear(Dbin1, Dbin2) | ||
self.assertArrayNear(Dbin1, Dbin1.T) |
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.
Testing that the viscosity and binary diffusion coefficients are the same as returned by the Mix
model is probably unnecessary.
* fraction. | ||
* | ||
* \f[ | ||
* D_{m} = \frac{\lambda}{\rho c_p} |
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 should still be D^\prime_{km}
if we're being consistent with Kee's notation, right?
{ | ||
public: | ||
//! Default constructor. | ||
UnityLewisTransport() {} |
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.
No need to define the constructor if it doesn't do anything.
const doublereal lambda = this->thermalConductivity(); | ||
const doublereal rho = this->m_thermo->density(); | ||
const doublereal cp = this->m_thermo->cp_mass(); | ||
const doublereal Dm = lambda/(rho*cp); |
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.
No need to explicitly reference this->
(here or anywhere else).
I think it would actually be more clear to just write this as
double Dm = thermalConductivity() / (m_thermo->density() * m_thermo->cp_mass());
/*! | ||
* | ||
* @ingroup tranprops | ||
*/ |
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.
The @ingroup tranprops
directive can be on just another line starting with //!
rather than also starting a /*
-style block.
ee78011
to
5877f18
Compare
5877f18
to
ad31fd7
Compare
Codecov Report
@@ Coverage Diff @@
## master #510 +/- ##
==========================================
+ Coverage 64.74% 64.75% +<.01%
==========================================
Files 383 384 +1
Lines 40689 40699 +10
==========================================
+ Hits 26345 26354 +9
- Misses 14344 14345 +1
Continue to review full report at Codecov.
|
Please fill in the issue number this pull request is fixing:
Fixes #507
Changes proposed in this pull request:
Supersedes pull requests #427 and #508 . The current implementation requires only minimal code changes and has been tested. The extension of the unity Lewis number transport model to a per-species Lewis number transport model is rather straight forward, but should be done in an own class.