New SA version/correction combinations and new way of specifying them in the config (SA_OPTIONS=...) #1646
Conversation
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
TestCases/transition/Schubauer_Klebanoff/transitional_BC_model_ConfigFile.cfg
Outdated
Show resolved
Hide resolved
| NONE, /*!< \brief No option / default. */ | ||
| NEG, /*!< \brief Negative SA. */ | ||
| EDW, /*!< \brief Edwards version. */ | ||
| FT2, /*!< \brief Use FT2 term. */ |
There was a problem hiding this comment.
I think it's better to use the keyword NOFT2 instead of FT2 to be more compatible with the NASA naming.
There was a problem hiding this comment.
Same problem as with "modified" "unmodified" if we want to default to noft2 we need two options, and one will be redundant
There was a problem hiding this comment.
But we can change to WITHFT2 to avoid confusion
There was a problem hiding this comment.
For compatibility with previous config files and avoid problems with transition, see #1066 (comment), I think it is better to set the default to the no-ft2 version and add activate it with WITHFT2.
There was a problem hiding this comment.
I think the nasa website should be leading here. If they call it SA-NOFT2, then I think for the end user it is most clear if we use SA_OPTIONS=NOFT2
Also, if the user sees in the config file that the turbulence model is SA, then it should be SA, and not SA-neg, or SA-noft2.
We should prevent submodels being activated in a hidden way. Maybe it is even better to either force the user to provide an SA_OPTION, to make her aware of the change (my preference), or give a warning that no SA_OPTION is found (might be missed).
There was a problem hiding this comment.
I would agree with you Nijso, if we were doing it from scratch, but that boat sailed and we are not going to break backward compatibility like that.
We can think about it for V8.
But I have a good solution for the "legal" combinations problem, to use the "illegal" ones it will be necessary to put "EXPERIMENTAL" in the SA options.
Ok?
There was a problem hiding this comment.
Sure, we can postpone the 'breaking' when we have the power of a V8 and the users had some time to adjust.
And if a user really wants to use combinations that are not found in the literature, we can allow it with some warnings.
|
We are compiling those combinations we may well make them available, best I
can due is issue warnings.
…On Mon, 23 May 2022, 14:43 Nijso, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Common/include/option_structure.hpp
<#1646 (comment)>:
> + * \param[in] SA_Options - Selected SA option from config.
+ * \param[in] nSA_Options - Number of options selected.
+ * \param[in] rank - MPI rank.
+ * \return Struct with SA options.
+ */
+inline SA_ParsedOptions ParseSAOptions(const SA_OPTIONS *SA_Options, unsigned short nSA_Options, int rank) {
+ SA_ParsedOptions SAParsedOptions;
+
+ auto IsPresent = [&](SA_OPTIONS option) {
+ const auto sa_options_end = SA_Options + nSA_Options;
+ return std::find(SA_Options, sa_options_end, option) != sa_options_end;
+ };
+
+ const bool found_neg = IsPresent(SA_OPTIONS::NEG);
+ const bool found_edw = IsPresent(SA_OPTIONS::EDW);
+ const bool found_bsl = !found_neg && !found_edw;
⬇️ Suggested change
- const bool found_bsl = !found_neg && !found_edw;
+ const bool found_bsl = !found_neg && !found_edw;
+ const bool found_ft2 = IsPresent(SA_OPTIONS::FT2);
+
+ if (found_neg && !found_ft2) {
+ SU2_MPI::Error("Negative and NOFT2 were both selected but are incompatible. Please choose only one.", CURRENT_FUNCTION);
+ }
—
Reply to this email directly, view it on GitHub
<#1646 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJCOXN33ZH2GAQODQZ7M2BLVLODOVANCNFSM5WSMMMUA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| @@ -110,7 +110,7 @@ CTurbSASolver::CTurbSASolver(CGeometry *geometry, CConfig *config, unsigned shor | |||
|
|
|||
| Factor_nu_Inf = config->GetNuFactor_FreeStream(); | |||
| su2double nu_tilde_Inf = Factor_nu_Inf*Viscosity_Inf/Density_Inf; | |||
| if (config->GetKind_Trans_Model() == TURB_TRANS_MODEL::BC) { | |||
There was a problem hiding this comment.
This brings up an interesting possible issue. Once we have a LM transition model, where will the BC transition live? SA options? or will it be in TRANSITION_MODEL?
There was a problem hiding this comment.
Yep SA_OPTIONS I think, since it is a very simple modification that does not require an entire solver / extra equations.
TestCases/turbulence_models/sa/rae2822/turb_SA_COMP_EDW_RAE2822.cfg
Outdated
Show resolved
Hide resolved
|
Dejavu from modified and unmodified...
We want to default to noft2, a noft2 option is redundant.
…On Mon, 23 May 2022, 14:35 Nijso, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Common/include/option_structure.hpp
<#1646 (comment)>:
> @@ -1056,18 +1045,98 @@ inline SST_ParsedOptions ParseSSTOptions(const SST_OPTIONS *SST_Options, unsigne
return SSTParsedOptions;
}
+/*!
+ * \brief SA Options
+ */
+enum class SA_OPTIONS {
+ NONE, /*!< \brief No option / default. */
+ NEG, /*!< \brief Negative SA. */
+ EDW, /*!< \brief Edwards version. */
+ FT2, /*!< \brief Use FT2 term. */
I think it's better to use the keyword NOFT2 instead of FT2 to be more
compatible with the NASA naming.
—
Reply to this email directly, view it on GitHub
<#1646 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJCOXNZS5KCB7L45MPF4KULVLOCT3ANCNFSM5WSMMMUA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
We can't really roll out SST_OPTIONS without SA_OPTIONS... so here it is.
This implements what @suargi envisioned in #1364 by leveraging the SA "modularization" done in #1413