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

[WIP] Inconsistencies and improvements to SST model #2329

Open
wants to merge 45 commits into
base: develop
Choose a base branch
from

Conversation

rois1995
Copy link
Contributor

@rois1995 rois1995 commented Jul 25, 2024

Proposed Changes

Hi everyone,

I have found some inconsistencies with respect to the literature on the implementation of the Menter's SST model. I would like to use this branch as test bench for any corrections/improvements made to the SST model.

Implementation errors found:

  • Use of production limiter constant in the clipping of the cross-diffusion term for the computation of the F1 blending function in CTurbSSTVariable.cpp.
  • SST naming conventions (V in V2003m should stay for Vorticity production term.). Not yet assessed.
  • Wrong cross-diffusion term included into the residual of w in turb_sources.hpp. It should not be only the positive value as considered in the computation of the F1 blending function. As stated in https://doi.org/10.2514/3.12149. "CDkw is the
    positive portion of the cross-diffusion term in Eq (A2)" pag. 1604. Moreover, a clipping has been introduced for large negative values of this term, as suggested in Peng, Shia-Hui, Peter Eliasson, and Lars Davidson. "Examination of the shear stress transport assumption with a low-Reynolds number k-omega model for aerodynamic flows." Eq 17.
  • Boundary conditions errors at inlets, following the Issue Turbulent Kinetic Energy(TKE) on energy equation in SST model. #1851. A fix has been proposed following @pcarruscag suggestions for the supersonic inlet BC.
  • Boundary conditions definitions are different than the ones proposed in TMR. Maybe give the user the choice to use the BCs implemented in SU2 or the ones from TMR.

Changes to SST model proposed:

  • Inclusion of non modified versions of SST. In this case I think that more work is needed to be sure that the correct terms are taken into account everywhere in the code.
  • Give the user the possibility of changing the production limiter for TKE (not essential).
  • Change values of default turbulent to laminar viscosity to 1e-2 (NASA TMR reports that it should be in the range of 1e-2 to 1e-5, but in SU2 this is set to 10 as default).

I've seen the proposed changes to the lower limits of k and w in #2323 and I tried implementing it in my branch. However, if the implementation proposed in the respective PR is preferred then I will change mine.

Related Work

#2323 #1851

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.

@rois1995 rois1995 changed the title Inconsistencies and improvements to SST model [WIP] Inconsistencies and improvements to SST model Jul 25, 2024
Comment on lines 859 to 860
nPrandtl_Lam, /*!< \brief Number of species
addDoubleOption("FREESTREAM_TURB2LAMVISCRATIO", TurbIntensityAndViscRatioFreeStream[1], 10.0); Prandtl number. */
Copy link
Contributor

@bigfooted bigfooted Jul 25, 2024

Choose a reason for hiding this comment

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

Was this intentionally commented, or...? If it is used, I guess this should go to line 872

Suggested change
nPrandtl_Lam, /*!< \brief Number of species
addDoubleOption("FREESTREAM_TURB2LAMVISCRATIO", TurbIntensityAndViscRatioFreeStream[1], 10.0); Prandtl number. */
nPrandtl_Lam, /*!< \brief Number of species laminar Prandtl number. */
addDoubleOption("FREESTREAM_TURB2LAMVISCRATIO", TurbIntensityAndViscRatioFreeStream[1], 10.0); /*!<\brief Freestream mu_turb to mu_lam viscosity ratio */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a wrong copy-paste from me. It should not be there in the first place.

@bigfooted
Copy link
Contributor

Great contribution, Thanks @rois1995 !

@pcarruscag
Copy link
Member

If you are looking into robustness aspects too you should get in touch with @emaberman and @YairMO, seems like they have some good ideas and between the free time of 3 people a lot more can get done :)

@YairMO
Copy link

YairMO commented Jul 25, 2024

Hi,

Regarding the cross-diffusion term (CD) that appears in Omega source term (residual). The SST model (1994/2003) is a high-Reynolds-number model. Namely, It can not predict correctly the sub-layer region (especially the correct profile of the TKE). Therefore, only a positive contribution is required. Moreover, since the SST model was design as a k-w and k-epsilon blending, the CD term "belongs" only to the k-epsilon "branch", that is why the CD term include the factor "1-F1". However, it may happen, that the factor "1-F1" is not a 100% safe guarantee. It may happen that "1-F1" is not zero in region where the CD term is negative (this happen due numerical errors). To avoid such a situation, it is a good idea to clip the CD term with zero. Otherwise, severe numerical robustness issues may rise.
Yes, it is different from Menter publications, but I think that clipping the CD term with zero is completely inline with Menter original idea (that is why, I think, he was including the factor "1-F1". But again the 1-F1 factor is not 100% percent "safe").

- Given option for cross diffusion limiting in W residual
@YairMO
Copy link

YairMO commented Jul 26, 2024

Hi,

The use of an Omega production limiter (about the cross-diffusion term) is correct for low-Reynolds-number (LRN) models (the approach described by Peng et al. is very naive; there are other more rigorous treatments). For high-Reynolds-number (HRN) models, the clipping should be zero, keeping the cross-diffusion term positive; thus, the current implementation is correct.

Indeed, it is not exactly as it appears in Menter's original publication. The factor (1-F1) aimed to promise that the cross-diffusion term will be activated only outside the boundary layer, where it is positive (the cross-diffusion term switches its sign deep inside the boundary layer). This was also recognized by Peng et al. (first paragraph above Eq. 17). However, it may happen that the factor (1-F1)=1 where the cross-diffusion term is negative. Usually, it may happen at the wake, very near the airfoil trailing edge, where the upper and lower boundary layers merge. It is due to the imperfection of the F1 function.

To summarize, the current implementation is correct, and it is perfect for HRN models.

@YairMO
Copy link

YairMO commented Jul 26, 2024

For the sake of clarity, "current implementation" refers to the current treatment of the production code

@emaberman
Copy link
Contributor

What YairMO is saying, is that allowing negative cross diffusion values is incorrect for high Reynolds models and should not be an option, this is a fix used for low Reynolds models only

@rois1995
Copy link
Contributor Author

rois1995 commented Jul 26, 2024

Hi @YairMO, Hi @emaberman ,

thank you very much for your comments. I haven't found any suggestion in literature to clip to only positive values the cross-diffusion term in the w-equation. I understand that it might be more robust, but it is not the standard implementation of the SST model, which is the first thing that we need to achieve. Only then we can build on top of that to improve the robustness of SU2.

Nevertheless, I tried the SWBLI test case and I compared the results across 6 different combinations:

1- develop branch, no changes
2- develop branch, changes to Supersonic_inlet profile as suggested in #1851
3- my branch, with original CDkw implementation (should give exactly the same result as develop+modified BC)
3- my branch, with original CDkw implementation and using boundary conditions from TMR
4- my branch, with original CDkw implementation and using your suggestions for lower limits for k and w.
5- my branch, allowing negative values of CDkw
6- my branch, allowing negative values of CDkw and using boundary conditions from TMR
7- my branch, allowing negative values of CDkw and using your suggestions for lower limits for k and w.
8- my branch, allowing negative values of CDkw, using boundary conditions from TMR and using your suggestions for lower limits for k and w.

When my branch is used, then the changes to the supersonic inlet BC are already in place.

I haven't achieved convergence with 1, 2 and 3. More precisely, 1 diverged right away (after 30 iterations), while 2 and 3 gave "FGMRES - Orthogonalization Failed" after 900ish iterations.

Here you can see the residuals for the different combinations.

OrigCDkw

NegCDkw

Unfortunately I will be busy with the AIAA Conference next week, thus I don't know how much I will be able to work on this. The next test case will be the 2D airfoil near-wake from TMR.

@YairMO
Copy link

YairMO commented Jul 26, 2024

Hi rois1995,

First of all, enjoy your time in Las Vegas. Any paper that you are presenting?

As for our discussion about the cross-diffusion term, I've emailed the "source" (Menter). I believe he will make it clear.
It may be that he will be able to answer only in a while ...

@rois1995
Copy link
Contributor Author

The wall distance is correctly computed. The nu_tilde, however, is higher on the symmetry plane, just like the Mach number. And, again, it happens only at the lower symmetry boundary.

LowerSymmetrySA_NuTilde

@YairMO
Copy link

YairMO commented Sep 24, 2024

Based on your input (unexpected generation of the eddy viscosity at the lower symmetry BC), I would suspect the wall distance calculation. Can you tell how the wall distance is calculated in the cells above the symmetry section? Consider a cell above the symmetry section. Is its wall distance taken relative to the symmetry part or relative to the wall part?

@rois1995
Copy link
Contributor Author

It is taken relative to the wall part (which you can see as a straight white line. I don't know if the increase in nu_tilde causes an increase of the vertical velocity on the symmetry or it's the opposite.

LowerSymmetrySA_WD

@YairMO
Copy link

YairMO commented Sep 24, 2024

Yes, the WD is as it should be; thank you. I also agree: who comes first, the egg or the chicken? I'll try to run this case using my code.

@rois1995
Copy link
Contributor Author

rois1995 commented Sep 24, 2024

I think the problem is in the fix for the symmetry boundary in PR #2194. I tried with the develop at the last commit (LC), at the commit before the implementation of the correction (BC) and at the commit of the implementation (AC). It is clearly visible that something is happening with the symmetry BC correction. I will look further into it

CommitsComparison_RMSRho

Also the profiles of the Mach number and nu_tilde are the same between LC and AC. With the BC instead the Mach number and nu_tilde are correct.

@rois1995
Copy link
Contributor Author

I tried without the MUSCL reconstruction and with the AC commit and both Mach and nu_tilde profiles are correct at the symmetry plane. @bigfooted can you comment on this please?

@rois1995
Copy link
Contributor Author

I think I found out the problem. In the correction of the gradients on the symmetry plane, somehow the gradient of the velocity along the normal direction was still not 0. By fixing it then the Mach number and nu_tilde profile are correct. I don't know if I am still missing the correction in the tangential direction, but at least the solution improves.

Here you can see the same graph as before but without the line of the last commit. I have added, instead, a try with the Weighted Least Square (LS) and a first order (FO) (no MUSCL). In the end I have added the line for the correction implemented (Corr Grad). We can see that the behaviour before the implementation of the gradient correction is recovered.

CommitsComparison_RMSRho_New

// }
// }

// Do I really need all of the previous code? Can't I just merge the two for loops? Maybe I am missing the tangential direction
Copy link
Member

Choose a reason for hiding this comment

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

Yes what you are doing neglects that velocity is a vector quantity

Copy link
Contributor Author

@rois1995 rois1995 Sep 26, 2024

Choose a reason for hiding this comment

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

The previous implementation was actually correct, I'll revert to that one. Nevertheless something is off in the gradient computations since without MUSCL reconstruction I don't have these problems.

Copy link
Member

Choose a reason for hiding this comment

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

Can you try the coarser mesh and converging it very well to check if this problem still happens? In your previous plots the residuals are not dropping a lot.

@rois1995
Copy link
Contributor Author

rois1995 commented Sep 27, 2024

I tried running the coarsest mesh using the develop branch on the commit before the symmetry correction (BC, bb0f9de) and at the symmetry correction (AC, 77fed34). For lines blue and yellow I tried without NK for the first 1000 iterations and then with NK. Convergence is good but somewhat different. No strange behaviour on the Mach number and nu_tilde are present. Lines purple and red are with NK from the start. No convergence at all on the AC commit and bad solutions at the symmetry planes. Everything normal on the BC commit.

CommitsComparison_RMSRho_Grid1

I will try changing the cell spacing in the normal and tangential direction separately and check what brakes it.

@rois1995
Copy link
Contributor Author

rois1995 commented Sep 27, 2024

I've tried reducing the residuals up to -11.5 for the logarithm of the density residual. I've started on the coarsest grid as you saw in the previous plot. I've proceeded on the L2 mesh keeping either the tangential or the first cell spacing constant, using as restart the converged solution on the L1 mesh. The results are essentially the same as with L1 mesh. When using the NK solver straight away the AC branch does not converge, while the BC one easily converged. If I remove the NK solver, both converge. The same has happened on the L3 grid (restart from converged L2 results).

Here you can see the residuals for the L2 grid (either the case, he outcome is the same)

CommitsComparison_RMSRho_Grid2

No strange behaviors of Mach number and nu_tilde have been seen on converged solutions. On not-converged ones, instead, the problem is right at the symmetry planes.

@pcarruscag
Copy link
Member

Thanks for testing, part of the NK options is a period where instead of NK we use the normal quasi-Newton approach.
I can't give too much detail on this, but we lack a good startup strategy, ideas from papers on NK approached could be useful even for the normal solver.

@rois1995
Copy link
Contributor Author

But what do you think of the changes between the two commits? It doesn't seem that the convergence is improved when using the new symmetry BC implementation.

@YairMO
Copy link

YairMO commented Sep 27, 2024

Could you please share your L3 grid (together with your input file)?

@pcarruscag
Copy link
Member

The changes to the symmetry BC were to improve the accuracy, especially when there are walls perpendicular to the symmetry plane.
It's possible the Jacobians can be improved to be more consistent with the RHS and recover some of the convergence rate.

@rois1995
Copy link
Contributor Author

@YairMO in the zip file you can find the .geo file to create the mesh via gmsh, the L3 mesh and the configs file used for simulations with SA.
MeshAndConfig.zip

@rois1995
Copy link
Contributor Author

If you see the stagnation in residuals on the L1 meshes while not using the NK solver (line blue and yellow), it is due to a single point in the grid that you can see here. I checked the mesh quality in that point and it doesn't seem to be bad, or at least not worse than the other points.

StrangeResiduals

@YairMO
Copy link

YairMO commented Sep 28, 2024

Hi,

I'm using SU2 8.1.0 but with some differencing in SA implicit treatment (underwork).
I've run the L3 grid with two different input files that differ only by the number of linear solver iterations. Attached is the convergence of the density

SWBLI-SU2-Conv

@rois1995
Copy link
Contributor Author

rois1995 commented Sep 28, 2024

Impressive! Can you share some info on your implementation? What about the CFL values and the computational time?

One more thing: in another PR (if I recall correctly it was the one for the dimensionless limits to SST) you mentioned the VFE2 test-case. Do you have access to shareable CAD files and meshes?

@YairMO
Copy link

YairMO commented Sep 28, 2024

Thank you.
The input files that I've used are based on your file. As shown in the figure, mine differs only by the number of linear solver iterations.
As for the algorithm, I truly apologize; at this moment, I can't share this, but we will soon be able to (we are working on this). Nevertheless, my point in this exercise is to check in your simulation how the number of linear solver iterations affects the convergence of the non-linear problem, especially that of the 77fed34-NK run.

As for the VFE-2 mesh, I've obtained this mesh from someone else. Please let me check with him first (I believe there will be no problem. Sorry).

@rois1995
Copy link
Contributor Author

rois1995 commented Sep 28, 2024

Sure, thank you for checking!

Regarding the SWBLI case, if I understood correctly, you have used your code with NK solver from the start and you changed the number of linear solver iterations, right? Can you also try with SST or is your implementation different only for SA?

@YairMO
Copy link

YairMO commented Sep 28, 2024

Yes, your understanding is correct.
As for the SST ... checking

@YairMO
Copy link

YairMO commented Sep 28, 2024

Here are the results obtained using the SST2003 model

SWBLI-Conv-SST2003

@rois1995
Copy link
Contributor Author

It seems that yoru implementation affects also the SST model, is it correct? Nevertheless, huge improvement wrt both this branch and develop branch results (density residual stagnates around -10)!

@YairMO
Copy link

YairMO commented Sep 28, 2024

Yes, it also affects the SST model. Thanks

@YairMO
Copy link

YairMO commented Sep 29, 2024

Hi Rois1995,

I have permission to share the VFE-2 grid; please send me your private email.

@rois1995
Copy link
Contributor Author

That's perfect, thank you! [email protected]

@rois1995
Copy link
Contributor Author

rois1995 commented Oct 4, 2024

I have a few updates. I found out that most of my FGMRES problems in this branch where related to the fact that I was using single precision for the linear system. You can see it in this graphs for the residuals of Rho.

RMSRho_Mesh_3

The first two curves are in mixed-precision and they stop long before reaching the minimum residual required due to divergence of the linear solver.

Moreover, it seems that the 2003 model here implemented (which, simply put, considers the full reynolds stress tensor for the computation of the production of k and always considers k in the stress tensor and in the thermodynamic variables) has faster convergence and increases the recirculating zone (maybe due to the reduction of turbulence kinetic energy) which is a feature that has also been seen in [DOI:10.1017/aer.2020.93].

SFC_Mesh_3

There are some differences between the use of the TMR boundary conditions and the ones used before.

The results improve with respect to the develop in the prediction of the SFC distribution, which has results completely off from the V&V page of SU2

SFC_Mesh_3_Old

RMSRho_Mesh_3_Old

I may keep on checking if the implementation of the v2003 model is correct or not by searching for other test cases (probably coming from DOI:10.1017/aer.2020.93).

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.

5 participants