-
Notifications
You must be signed in to change notification settings - Fork 843
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] Fix symmetry plane issues in SU2, change SU2-NEMO sym plane to match flow solver #1635
Conversation
Signed-off-by: jtneedels <jneedels@stanford.edu>
Signed-off-by: jtneedels <jneedels@stanford.edu>
Signed-off-by: jtneedels <jneedels@stanford.edu>
Signed-off-by: jtneedels <jneedels@stanford.edu>
@@ -269,5 +269,19 @@ class CIncEulerVariable : public CFlowVariable { | |||
inline void SetVelSolutionVector(unsigned long iPoint, const su2double *val_vector) final { | |||
for (unsigned long iDim = 0; iDim < nDim; iDim++) Solution(iPoint, iDim+1) = val_vector[iDim]; | |||
} | |||
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to do this rather an than adding dummy functions to these variable classes? The compiler throws an error since NEMO requires these 3 functions, which don't exist in the other solvers. Would it be better to pass the variable type into the sym plane template explicitly somehow, like what is done for conv_numerics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable type is known in CFVMFlowSolverBase, but to do something conditionally on a type in C++11 you need to create a helper function that is specialized for the type (CNEMOEulerVariable in this case)
template <class VariableType>
void SetExtraVars(unsigned long, VariableType*, CNumerics*) {
// Do nothing for non-NEMO solvers.
}
template <>
void SetExtraVars<CNEMOEulerVariable>(unsigned long iPoint, CNEMOEulerVariable* nodes, CNumerics* numerics) {
// Template specialization for NEMO variables, here you set Eve, etc.
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcarruscag Do both the general template function declaration and the specialized declaration go in the inl file (i.e. implemented as you have it above? I get a compilation error of "unknown type name 'CNEMOEulerVariable'; did you mean 'CEulerVariable'?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try putting the specialization in CNEMOEulerSolver.h
if (nemo) { | ||
conv_numerics->SetEve (nodes->GetEve(iPoint), nodes->GetEve(iPoint)); | ||
conv_numerics->SetCvve (nodes->GetCvve(iPoint), nodes->GetCvve(iPoint)); | ||
conv_numerics->SetGamma (nodes->GetGamma(iPoint), nodes->GetGamma(iPoint)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use pretty formatting, we have a clang-format style
if (nemo) { | |
conv_numerics->SetEve (nodes->GetEve(iPoint), nodes->GetEve(iPoint)); | |
conv_numerics->SetCvve (nodes->GetCvve(iPoint), nodes->GetCvve(iPoint)); | |
conv_numerics->SetGamma (nodes->GetGamma(iPoint), nodes->GetGamma(iPoint)); | |
if (nemo) { | |
conv_numerics->SetEve(nodes->GetEve(iPoint), nodes->GetEve(iPoint)); | |
conv_numerics->SetCvve(nodes->GetCvve(iPoint), nodes->GetCvve(iPoint)); | |
conv_numerics->SetGamma(nodes->GetGamma(iPoint), nodes->GetGamma(iPoint)); |
@@ -269,5 +269,19 @@ class CIncEulerVariable : public CFlowVariable { | |||
inline void SetVelSolutionVector(unsigned long iPoint, const su2double *val_vector) final { | |||
for (unsigned long iDim = 0; iDim < nDim; iDim++) Solution(iPoint, iDim+1) = val_vector[iDim]; | |||
} | |||
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable type is known in CFVMFlowSolverBase, but to do something conditionally on a type in C++11 you need to create a helper function that is specialized for the type (CNEMOEulerVariable in this case)
template <class VariableType>
void SetExtraVars(unsigned long, VariableType*, CNumerics*) {
// Do nothing for non-NEMO solvers.
}
template <>
void SetExtraVars<CNEMOEulerVariable>(unsigned long iPoint, CNEMOEulerVariable* nodes, CNumerics* numerics) {
// Template specialization for NEMO variables, here you set Eve, etc.
...
}
Signed-off-by: jtneedels <jneedels@stanford.edu>
Signed-off-by: jtneedels <jneedels@stanford.edu>
Signed-off-by: jtneedels <jneedels@stanford.edu>
@@ -90,6 +90,9 @@ CNEMOEulerVariable::CNEMOEulerVariable(su2double val_pressure, | |||
eves.resize(nPoint, nSpecies) = su2double(0.0); | |||
Gamma.resize(nPoint) = su2double(0.0); | |||
|
|||
/* Vector to count number of symmetry planes at each node. */ | |||
symmetry.resize(nPoint) = su2double(0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be needed for the base and Inc solvers too, correct?
V_reflected[iDim + 1] = nodes->GetVelocity(iPoint, iDim) - 2.0 * ProjVelocity_i * UnitNormal[iDim]; | ||
} | ||
for (iDim = 0; iDim < nDim; iDim++) | ||
V_reflected[prim_idx.Velocity() + iDim] = nodes->GetVelocity(iPoint, iDim) - 2.0 * ProjVelocity_i * UnitNormal[iDim]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and it gave an error when running with nemo, which is why I didn't push it. Did you test and ensured this gave correct results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine, it didn't seem to have any issues. What issues did you see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case solution I was using gave the wrong solution when I used this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, I just double checked (after I fixed my compilation errors I caused). The value of the velocity index was 7 for AIR-5.
hi guys, what's the status of this WIP? By the way, does this also show changes (improvements I hope) in convergence, or is that not affected by the change |
Hi @bigfooted, I'm a bit perplexed because even with using the same sym plane BC in CFVMFlowSolverBase.inl as the standard air solver, the NEMO solver continues to have issues (pressure defects at stagnation point on sym boundary wall). The last working push in Fabio's original PR seem to fix these problems, but they involve some pretty significant changes to the code (complete change of BC, change of order of applications of BCs in CIntegration, etc.). I'm a bit stuck since I'm not sure why all these changes are necessary to get good results for NEMO, when the current implementation seems to work in CFVMFlowSolverBase.inl for the standard air solver. I was hoping to chat about this at the next developer meeting. |
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. |
|
||
/*--- Allocation of variables necessary for convective fluxes. ---*/ | ||
su2double Area, ProjVelocity_i, *V_reflected, *V_domain, Normal[MAXNDIM] = {0.0}, UnitNormal[MAXNDIM] = {0.0}; | ||
|
||
/*--- Allocation of variables necessary for viscous fluxes. ---*/ | ||
su2double ProjGradient, ProjNormVelGrad, ProjTangVelGrad, TangentialNorm, | ||
Tangential[MAXNDIM] = {0.0}, GradNormVel[MAXNDIM] = {0.0}, GradTangVel[MAXNDIM] = {0.0}; | ||
Tangential[MAXNDIM] = {0.0}, GradNormVel[MAXNDIM] = {0.0}, GradTangVel[MAXNDIM] = {0.0}, Residual[MAXNVAR] = {0.0}; |
Check notice
Code scanning / CodeQL
Unused local variable
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nijso's fix to the symmetry is in so I'm closing this PR.
I'll delete the branch next week unless someone wants to keep it.
Proposed Changes
This PR will help address issues seen in SU2 when symmetry planes are applied. These include pressure oscillations at sym plane & solid wall interfaces (i.e. blunt bodies). It will also change the implementation of the sym plane in SU2-NEMO to that used by the rest of the code, the template function in CFVMFlowSolverBase.inl.
Related Work
This PR will largely utilize the work done in #1168 by Fabio Morgado. It is in addition to #657. It will address several open issues #1625 and #1125.
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.