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] PGOMEGA Implementation #2354

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

Conversation

TurboLdd
Copy link

Proposed Changes

Hi all,
I have implemented the PGOmega S-A turbulence model(TM) which is proposed by Xiao He(2022). This TM enables us to get a more accurate prediction of stall margin which is very important in turbomachinery. This TM has been used in the simulations of ROTOR67, TUDarmstadt single stage compressor, and the validation work is shown in GPPS 2024 conference.
I also added the option related in configuration file.
The paper related is "He, X., Zhao, F., and Vahdati, M. (September 19, 2022). "A Turbo-Oriented Data-Driven Modification to the Spalart–Allmaras Turbulence Model."

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.

@TurboLdd TurboLdd marked this pull request as draft September 10, 2024 14:35
@TurboLdd TurboLdd marked this pull request as ready for review September 10, 2024 14:35
@joshkellyjak
Copy link
Contributor

Do you have a testcase that demonstrates this implementation? Can you reproduce and compare to the validation plots published by Xiao He? I think you have shared some similar results with us before.

Copy link
Contributor

@joshkellyjak joshkellyjak left a comment

Choose a reason for hiding this comment

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

If this is a turbo-oriented feature I think you should highlight this in the documentation when you update it

SU2_CFD/include/numerics/CNumerics.hpp Show resolved Hide resolved

/*--- List of auxiliary functions ---*/
su2double ft2, d_ft2, r, d_r, g, d_g, glim, fw, d_fw, Ji, d_Ji, S, Shat, d_Shat, fv1, d_fv1, fv2, d_fv2;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

su2double RefReynold = 1.0e6;
dpomg = GeometryToolbox::DotProduct(3, PrimVar_Grad_i[idx.Pressure()], Vorticity_Rel) * laminar_viscosity *
RefReynold / (omg * pow(velocityMag, 3) * pow(density, 2)); // reynold should be modified
//cout<<"dpomg="<<dpomg<<endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comments

@@ -42,6 +42,7 @@ class CTurbSAVariable final : public CTurbVariable {
VectorType DES_LengthScale;
VectorType Vortex_Tilting;


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -87,4 +88,5 @@ class CTurbSAVariable final : public CTurbVariable {
*/
inline su2double GetVortex_Tilting(unsigned long iPoint) const override { return Vortex_Tilting(iPoint); }


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1413,6 +1413,7 @@ void CFlowOutput::SetVolumeOutputFieldsScalarPrimitive(const CConfig* config) {

if (config->GetKind_Turb_Model() != TURB_MODEL::NONE) {
AddVolumeOutput("EDDY_VISCOSITY", "Eddy_Viscosity", "PRIMITIVE", "Turbulent eddy viscosity");

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -1544,6 +1545,7 @@ void CFlowOutput::LoadVolumeDataScalar(const CConfig* config, const CSolver* con
SetVolumeOutputValue("EDDY_VISCOSITY", iPoint, Node_Flow->GetEddyViscosity(iPoint));
SetVolumeOutputValue("TURB_DELTA_TIME", iPoint, Node_Turb->GetDelta_Time(iPoint));
SetVolumeOutputValue("TURB_CFL", iPoint, Node_Turb->GetLocalCFL(iPoint));

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

velocityMag = max(GeometryToolbox::Norm(3, Velocity_Rel), 0.001);
omg = max(GeometryToolbox::Norm(3, Vorticity_Rel), 0.001);

for (int iDim = 0; iDim < nDim; iDim++) dpds += PrimVar_Grad_i[idx.Pressure()][iDim] * Velocity_Rel[iDim];
Copy link
Contributor

Choose a reason for hiding this comment

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

Check capitialisation for consistency, should this be dP_ds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same with line 162

Velocity_Rel[iDim] = V_i[idx.Velocity() + iDim] - RotationVelocityCrossR[iDim];
Vorticity_Rel[iDim] = Vorticity_i[iDim] - RotationalVelocity[iDim];

}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this model only works for 3D flows (I assume you only use it in these cases) you should not allow users to select it with a 2D calculation

su2double Velocity_Rel[3] = {0.0, 0.0, 0.0};
su2double Vorticity_Rel[3] = {0.0, 0.0, 0.0};
su2double RotationalVelocity[3] = {0.0, 0.0, 0.0};
su2double RotationVelocityCrossR[3] = {0.0, 0.0, 0.0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
su2double RotationVelocityCrossR[3] = {0.0, 0.0, 0.0};
su2double RotationVelocityCrossR[nDim] = {0.0};

Copy link
Contributor

Choose a reason for hiding this comment

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

or, since you are already doing static allocation:

Suggested change
su2double RotationVelocityCrossR[3] = {0.0, 0.0, 0.0};
su2double RotationVelocityCrossR[MAXNDIM];

and you do not rely on the initial value being 0 as far as I see

Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

Hey @TurboLdd , I see this is your first PR, so congrats to that :)

I am with Josh there that it would be great to have the validation plots created with this PR-code, and in addition the setup of that added as a small Test or unit-tested.

Concerning the implementation, from my pov it is fine to first get a working version and then make it work with the existing structure - and for that I think you make it easier on yourself if you have a small test readily set up to iterate on, once you start refactoring.

su2double Velocity_Rel[3] = {0.0, 0.0, 0.0};
su2double Vorticity_Rel[3] = {0.0, 0.0, 0.0};
su2double RotationalVelocity[3] = {0.0, 0.0, 0.0};
su2double RotationVelocityCrossR[3] = {0.0, 0.0, 0.0};
Copy link
Contributor

Choose a reason for hiding this comment

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

or, since you are already doing static allocation:

Suggested change
su2double RotationVelocityCrossR[3] = {0.0, 0.0, 0.0};
su2double RotationVelocityCrossR[MAXNDIM];

and you do not rely on the initial value being 0 as far as I see

@@ -1113,6 +1113,7 @@ enum class SA_OPTIONS {
ROT, /*!< \brief Rotation correction. */
BC, /*!< \brief Bas-Cakmakcioclu transition. */
EXP, /*!< \brief Allow experimental combinations of options (according to TMR). */
PGOMGA, /*!< \brief Use PGOMGA term. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the DOI address of your reference somewhere (maybe here), i.e. some https://doi.org/<the DOI string>address

@@ -283,7 +323,7 @@ struct Omega {
struct Bsl {
template <class MatrixType>
static void get(const su2double* vorticity, unsigned short, const MatrixType&, CSAVariables& var) {
var.Omega = GeometryToolbox::Norm(3, vorticity);
var.Omega = GeometryToolbox::Norm(3, vorticity)*(1.0 + var.beta_PGO);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not familiar with the PGOMGA model but my guess is that we can better fit it into the current modular structure than it is done here. E.g. under the struct Omega { you could create you own struct PGOMGA { and put down you omega contribution there.

Currently also in the ComputeResidual routine there is specific PGOMGA contributions which would be great to move into specialized PGOMGA contributions following the current structure

for (int iDim = 0; iDim < nDim; iDim++) dpds += PrimVar_Grad_i[idx.Pressure()][iDim] * Velocity_Rel[iDim];

if (dpds >= 0.0) {
su2double RefReynold = 1.0e6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
su2double RefReynold = 1.0e6;
const su2double RefReynold = 1.0e6;

const all the things possible :)

var.beta_PGO = var.cpw1 * tanh(var.cpw2 * pow(dpomg, var.cpw3));
}

//var.Omega = omg * (1.0 + var.beta_PGO);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@pcarruscag pcarruscag changed the title PGOMEGA Implementation [WIP] PGOMEGA Implementation Sep 14, 2024
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.

3 participants