Closed economon closed 4 years ago
May I suggest a different approach? Formally the text above is correct. However, the ft2 term was originally introduced to allow a transition to occur. Not so surprisingly it is not an accurate mechanism for transitional flow. To the best of my recollection, Philippe Spalart also acknowledged it, so during the years that followed 1992, the ft2-less SA model became actually the standard. We should remember that a newbie to the code is likely to opt for the standard model. I suggest leaving this one as is, add a comment to the documentation and add a model with ft2 option. While this doesn't match the formal naming at the larc web page (which is an excellent resource for models versions) it is user friendly
The original Spalart-Allmaras turbulence model encloses the ft2 term: https://www.researchgate.net/publication/236888804_A_One-Equation_Turbulence_Model_for_Aerodynamic_Flows or https://turbmodels.larc.nasa.gov/spalart.html#sa. After that, some modifications were introduced and the ft2 term was neglected. But accordingly, a different name is used, i.e., Spalart-Allmaras One-Equation Model without ft2 Term. So, I would also vote for implementing the original model along with the SA-noft2 variant.
Also, I would like to point out a fact about the current implementation of the Negative Spalart-Allmaras variant.
From theory, https://www.iccfd.org/iccfd7/assets/pdf/papers/ICCFD7-1902_paper.pdf equation 12, the model introduces the modified vorticity S_tilde. However, in SU2 (SU2/SU2_CFD/src/numerics/turbulent/turb_sources.cpp and CSourcePieceWise_TurbSA_Neg::ComputeResidual) we do not consider this modification and simply consider the modified vorticity as in the standard Spalart-Allmaras:
Shat = S + TurbVar_i[0]fv2inv_k2_d2;
(Sbar is used as S_tilde)
Is there any reason for that?
I think I found another issue, the QCR stress tensor (https://turbmodels.larc.nasa.gov/spalart.html#qcr2000) is computed as:
for (iDim = 0 ; iDim < nDim; iDim++){
for (jDim = 0 ; jDim < nDim; jDim++){
for (kDim = 0 ; kDim < nDim; kDim++){
O_ik = (val_gradprimvar[iDim+1][kDim] - val_gradprimvar[kDim+1][iDim])/ den_aux;
O_jk = (val_gradprimvar[jDim+1][kDim] - val_gradprimvar[kDim+1][jDim])/ den_aux;
tau[iDim][jDim] -= c_cr1 * ((O_ik * tau[jDim][kDim]) + (O_jk * tau[iDim][kDim]));
}
}
}
The matrix multiplication above does not inspire confidence.
In general, I think we need to tighten up a bit on our implementation of the turbulence models and be a bit more rigorous. I feel that we should go by-the-book with the implementations and name each model accordingly, but then of course, we can pick reasonable defaults (such as the sanoft2 discussed above - which @erangit made a nice point about).
@suargi : as you are currently digging through the details of the implementations, I think you are in best position to propose some modifications to the turbulence models as you see fit, if you have the time.
I can do that. I will follow the above mentioned papers to implement the original SA and SA_NEG. So we will end up with SA, SA_NEG along with SA_NOFT2 and SA_NEG_NOFT2**.
SA_NEG_NOFT2**:
we do not consider this modification and simply consider the modified vorticity as in the standard Spalart-Allmaras
Food for thought: According to his most recent AIAA talk, Spalart himself has tried to keep the model variants "modular." Some of the variants are compatible with each other. For example, you can add a "rotation-curvature correction" and a "compressiblity correction". The NASA TMR catalogue reflects this design by stating "These corrections can be applied individually or together in combination with the General Model."
A simple SA_QCR
or SA_COMP
naming scheme doesn't match the underlying design. On the user-facing side, separate config options might be better for some of the variations. On the code side, bit flags (Issue #770) might be a good way to gather all the model variants together into a single config option.
@fpalacios: I am adding you to this discussion because I have seen you are the responsible of implementing the Negative Spalart-Allmaras variation in SU2. Maybe you can shed some light into the discussion. In particular to:
we do not consider this modification and simply consider the modified vorticity as in the standard Spalart-Allmaras
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still a relevant issue please comment on it to restart the discussion. Thank you for your contributions.
Describe the bug A clear and concise description of what the bug is and what you expect the behavior to be instead. If applicable, add screenshots to help explain your problem.
It was brought to our attention that what we currently define as the "Standard" SA model is not using the ft2 term in the source implementation:
https://github.com/su2code/SU2/blob/20888f488752e4e8aaa5230e56c5417923c7c344/SU2_CFD/src/numerics/turbulent/turb_sources.cpp#L137
which means it is actually the SA-noft2 variant:
https://turbmodels.larc.nasa.gov/spalart.html#sanoft2
In practice, this should make little difference for most problems, but it can be important for DES/DDES (where the ft2 term is typically avoided). However, it is also important that we be clear about the different variants of the turbulence models to users, since having confidence in the models for V&V, reproducibility, etc., is so critical.
I would vote that we reinstate the "standard" model and make a new SA variant for SA-noft2 as a separate option (without duplicating a ton of code, if possible). Anyone else feel strongly about this topic?