Skip to content
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

Change lower limit of SST variables #2323

Merged
merged 17 commits into from
Aug 5, 2024

Conversation

emaberman
Copy link
Contributor

@emaberman emaberman commented Jul 11, 2024

Proposed Changes

To improve robustness of the SU2 code, specifically when using the SST (1994/2003) model, we would like to suggest a slight modification to the lower limits of the turbulent kinetic energy (TKE) and the specific turbulent dissipation rate (Omega).

Presently, the lower limits are given as follows:
TKE_LoweLimit = 1E-10
Omega_LowerLimit = 1E-4

Estimating if these lower limits are sufficiently low is very difficult.
They may be too high. This is certainly the case whether one runs in a dimensional or non-dimensional mode. It may be sufficiently low for one mode but marginally low for the second mode. Therefore, we suggest clipping the solution relative to the TKE and Omega freestream values such that:

TKE_LowerLimit = C_k * TKE_infinity
Omega _LowerLimit = C_w * Omega_infinity

Where C_k and C_w are constants.

The control over these constants is given to the user, However based on our 20 years of experience with the k-w models, Omega will not drop below 1E-3 * Omega_infinity (converged solution). However, in the process of convergence, it may reach lower values. Therefore, a value of

C_w = 1E-6

is reasonable. The solution of TKE is not very sensitive to C_K (assume it is sufficiently low). We recommended:

C_K = 1E-20

setting these as default values.

Related Work

Results of the change on convergence of VFE2 test case are presented. Two configurations of CFL adaptation were tested on both the original and modified lower limit code,

  1. CFL_ADAPT_PARAM= ( 0.99, 1.005, 5.0, 50.0, 0.0001 ) named max CFL 50
  2. CFL_ADAPT_PARAM= ( 0.99, 1.05, 5.0, 200.0, 0.0001 ) named max CFL 200

rms Rho

rms k

we point out that beyond the change in maximum CFL, also the acceleration rate is changed.

Beyond adding robustness allowing the the use of a CFL of 200 which diverges under the original limit , the modified limit allows for better convergence of the flow and turbulence residuals for CFL of 50. Plots of the Lift and drag Coefficients are added here showing that results are not affected by the new limit, and additionally convergence is slightly quicker for the lift coefficient even with a CFL of 50 .

CL
CD

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

@bigfooted
Copy link
Contributor

Hi, Thank you for doing this research. It is research like this that will really get us the robustness that we would like to see in SU2.

However, when users select "SST 1994" or "SST 2003", they should get exactly what they asked for. Therefore, the default settings should be such that the user get the constants and model exactly as described in the papers of Menter. Can you make sure of this?
This could be done for instance by introducing your modification as an option to SST_OPTIONS.

@emaberman
Copy link
Contributor Author

It should be emphasized that we do not modify nor change the model formulation (terms, constants, coefficients, etc.) The Lower limit clipping of K and omega used currently in SU2 are not a part of the model formulation. It is a numerical fix to avoid negative values. To the best of our knowledge there is no formal lower limit given by Mentor or anyone else and current values in SU2 are taken arbitrarily. Based on our twenty years of experience, using the lower limit suggested will lead to a more robust scheme without compromising the quality of results. This has been demonstrated in the attached figures .

@bigfooted
Copy link
Contributor

Can you modify the default such that nothing changes for the existing test cases? You can then add a new test case to the repository with the modified limits.
As users get more familiar with this feature, this feature then be gradually introduced into other testcases and tutorials. It is also important to add some documentation to the website.

@emaberman
Copy link
Contributor Author

Our proposed changes implicitly include 2 modifications to current code:
The first is the modification of the lower limit such that it should be relative to a reference value. The rational for this is that a single given hard coded number cannot be correct for possible all flow regimes. Moreover, even running the same case in dimensional and non dimensional form, will lead to different behaviour of the lower limit. The proposed solution therefore is defining a limit relative to reference values, namely:
k_low_limit = C_k * K_infinity
w_low_limit = C_w * W_infinity

This change will inevitably cause changes to current behaviour (as can be seen in test case results), but we believe it is more correct.

The second, is giving the user control over C_k and C_w and the choice of the default value. As mentioned above, the behaviour of the proposed default value will inevitably be different than the existing one. Even if we choose default values that will lead to similar values to those existing for a certain case this would not be true for other cases. Therefore no default value can be given to reconstruct the current solver behaviour.
The values suggested as default are not the most correct per se, but rather robust suggestions based on experience and are open to discussion. Optimal default values of different cases, say external and internal flows are expected to be different, this is why we felt it important to allow user control of the lower limit.

We emphasise that the solution used in SU2 of applying a lower limit to turbulence variables in order keep the solution positive is somewhat artificial, and future features we are working on should minimize the need for such a treatment, however, as long as this is the solution implemented, we believe that the proposed change is more correct, leading to more robust behaviour.

@emaberman
Copy link
Contributor Author

After choosing final default values and updating test case results, we will add some documentation.

@YairMO
Copy link

YairMO commented Jul 16, 2024

Hi,

I am working with Eitan ("emaberman") about this issue (lower limit).

I want to strengthen a point mentioned by Eitan.
Using a specific non-dimension form available in the SU2, the hard-coded low limit values may be too high. In fact, they may interrupt the solution to reach a realizable lower value. On the other hand, using a dimension form, the current low limit is extremely low, which may hinder the convergence rate or, in the worst case, result in the solver diverging (as shown in the plots).

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SST is usually not very sensitive to freestream values because they decay very rapidly.
That is why they need to be set much more carefully for SST-SUST. Do you know if these factors are appropriate for that variant?

Regarding the non dimensionalization issue, would it be possible to non dimensionalize the current lower bounds? (and upper bounds too perhaps).

I agree with Nijso that we should not make this the default immediately. Also just due to practical reasons, we have a PR that already required updates to many cases and updating all the SST cases again is going to be a pain.

Common/include/CConfig.hpp Outdated Show resolved Hide resolved
Common/include/CConfig.hpp Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Outdated Show resolved Hide resolved
@YairMO
Copy link

YairMO commented Jul 16, 2024

Our proposal does not relate to the SST's good behavior (iThe SST model is not sensitive to Omega freestream value: it is because of the addition of the cross-diffusion term, which was absent in the original Wilcox model). The current implicit treatment in the SU2 may generate a negative solution that is not physical. A low limit value is available in the code to avoid a negative solution. This means the solution is clipped to this value if the solution reaches a lower value. There is no single absolute low-limit value that is suitable, for example, issues with dimension vs. non-dimension form. Our proposal circumvents this problem. The current situation in the code is not robust. We suffer from many robustness issues, especially with the SST2003 model. However, using our proposal significantly enhanced the solver's robustness. The current situation is like a "bug" because it may interfere with the solution.

We understand that our idea requires a lot of effort and that it would make the code unreproducible. Therefore, we would like to suggest the following: Give the user an option to control these low-limit values. The default values will be according to the hard-coded values. We also strongly recommend adding additional user low-limit values control according to our proposal. The user will be able to decide which option he prefers.

Our end goal is to design the implicit treatment so that these low-limit controls will be almost redundant. Based on several publications I've made during the last 15-20 years, my code enjoys this feature.

@pcarruscag
Copy link
Member

I understand. So you could add an SST option to use this way of defining the lower bounds, as an alternative to the fixed values we have.
I would be interested in reading about your findings, can you point me to some papers? Thanks.

@YairMO
Copy link

YairMO commented Jul 16, 2024

I'll be glad to share some of my publications with you. Some of the ideas presented in these papers have already been successfully implemented in our SU2 version, but it takes some time to push them. Could you write me in your email? Many thanks for considering our proposal.

@pcarruscag
Copy link
Member

pcarruscag commented Jul 16, 2024

Thanks, it's the same as my github handle @g...

@YairMO
Copy link

YairMO commented Jul 17, 2024

I've sent a few papers

@emaberman emaberman marked this pull request as ready for review July 22, 2024 09:17
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Can you update one SST regression case to use this option so that it's tested? thanks.

@pcarruscag pcarruscag merged commit 476acd3 into su2code:develop Aug 5, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants