Skip to content

Conversation

@bigfooted
Copy link
Contributor

Proposed Changes

Give a brief overview of your contribution here in a few sentences.

correction of eq. 7 and 8 in:

https://www.researchgate.net/publication/387963120_Enhancing_Accuracy_in_Mixed-Element_Grids_and_Convergence_on_Skewed_grids_for_the_Two-Dimensional_Edge-Based_Compressible_Navier-Stokes_Solver

alpha damping from Nishikawa:

https://www.researchgate.net/publication/318535481_Effects_of_High-Frequency_Damping_on_Iterative_Convergence_of_Implicit_Viscous_Solver

Taken from the branch feature_2ndOrderMixedGrids

  • 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.

Comment on lines +676 to +687
switch (correct) {
case VISCOUS_GRAD_CORR::EDGE_NORMAL:
diss = proj_vector_ij / max(dist_ij_2,EPS*EPS);
break;
case VISCOUS_GRAD_CORR::FACE_TANGENT:
diss = Area2 / max(proj_vector_ij,EPS);
break;
case VISCOUS_GRAD_CORR::ALPHA_DAMPING:
const su2double alpha = 4.0 / 3.0;
diss = alpha * Area2 / max(abs(proj_vector_ij),EPS);
break;
}

Check warning

Code scanning / CodeQL

Missing enum case in switch Warning

Switch statement does not have a case for
NONE
.

Copilot Autofix

AI about 1 month ago

To fix the problem, all enum members of VISCOUS_GRAD_CORR used in the switch statement should be handled explicitly or, alternatively, a default case should be added. The best way to proceed here is to add a case for VISCOUS_GRAD_CORR::NONE to document intent and control the handling for this value. If NONE should have specific behavior, provide the logic; otherwise, you can make it explicit that nothing should be done (e.g., leave diss at 0.0). Adding a comment may further clarify intent.

Make this change directly in the ComputeProjectedGradient function, at the switch statement on correct (lines 676–687).


Suggested changeset 1
SU2_CFD/include/numerics/CNumerics.hpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/include/numerics/CNumerics.hpp b/SU2_CFD/include/numerics/CNumerics.hpp
--- a/SU2_CFD/include/numerics/CNumerics.hpp
+++ b/SU2_CFD/include/numerics/CNumerics.hpp
@@ -681,9 +681,12 @@
         diss = Area2 / max(proj_vector_ij,EPS);
         break;
       case VISCOUS_GRAD_CORR::ALPHA_DAMPING:
-        const su2double alpha = 4.0 / 3.0;        
+        const su2double alpha = 4.0 / 3.0;
         diss = alpha * Area2 / max(abs(proj_vector_ij),EPS);
         break;
+      case VISCOUS_GRAD_CORR::NONE:
+        // No correction applied; diss remains 0.0
+        break;
     }
 
     //proj_vector_ij /= max(dist_ij_2,EPS);
EOF
@@ -681,9 +681,12 @@
diss = Area2 / max(proj_vector_ij,EPS);
break;
case VISCOUS_GRAD_CORR::ALPHA_DAMPING:
const su2double alpha = 4.0 / 3.0;
const su2double alpha = 4.0 / 3.0;
diss = alpha * Area2 / max(abs(proj_vector_ij),EPS);
break;
case VISCOUS_GRAD_CORR::NONE:
// No correction applied; diss remains 0.0
break;
}

//proj_vector_ij /= max(dist_ij_2,EPS);
Copilot is powered by AI and may make mistakes. Always verify output.
break;
}

//proj_vector_ij /= max(dist_ij_2,EPS);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

Copilot Autofix

AI about 1 month ago

To fix the problem, we should remove the commented-out code on line 689: //proj_vector_ij /= max(dist_ij_2,EPS);. We should not replace it with any alternative code nor attempt to integrate it unless its function is necessary, as there is no indication in the comment or surrounding code that it should be reinstated or has a continuing purpose. The change only involves deleting the line, and no additional definitions, imports, or method changes are required. Only SU2_CFD/include/numerics/CNumerics.hpp, at the flagged location, needs to be updated.


Suggested changeset 1
SU2_CFD/include/numerics/CNumerics.hpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/include/numerics/CNumerics.hpp b/SU2_CFD/include/numerics/CNumerics.hpp
--- a/SU2_CFD/include/numerics/CNumerics.hpp
+++ b/SU2_CFD/include/numerics/CNumerics.hpp
@@ -686,7 +686,6 @@
         break;
     }
 
-    //proj_vector_ij /= max(dist_ij_2,EPS);
 
     /*--- Mean gradient approximation. ---*/
     for (int iVar = 0; iVar < nVar; iVar++) {
EOF
@@ -686,7 +686,6 @@
break;
}

//proj_vector_ij /= max(dist_ij_2,EPS);

/*--- Mean gradient approximation. ---*/
for (int iVar = 0; iVar < nVar; iVar++) {
Copilot is powered by AI and may make mistakes. Always verify output.
const su2double gasConst;
const su2double prandtlTurb;
const bool correct;
//const su2double cp;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

Copilot Autofix

AI about 1 month ago

The best way to fix this problem is to remove the commented-out code line //const su2double cp; entirely. This avoids introducing confusion for future readers and adheres to clean code principles. No additional imports, method definitions, or code changes are required. Only the specific line in the file needs to be deleted, with surrounding context left unchanged.

Suggested changeset 1
SU2_CFD/include/numerics_simd/flow/diffusion/viscous_fluxes.hpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/include/numerics_simd/flow/diffusion/viscous_fluxes.hpp b/SU2_CFD/include/numerics_simd/flow/diffusion/viscous_fluxes.hpp
--- a/SU2_CFD/include/numerics_simd/flow/diffusion/viscous_fluxes.hpp
+++ b/SU2_CFD/include/numerics_simd/flow/diffusion/viscous_fluxes.hpp
@@ -77,7 +77,6 @@
   const su2double gamma;
   const su2double gasConst;
   const su2double prandtlTurb;
-  //const su2double cp;
   const bool correct_EN;
   const bool correct_FT;
   const bool correct_AD;
EOF
@@ -77,7 +77,6 @@
const su2double gamma;
const su2double gasConst;
const su2double prandtlTurb;
//const su2double cp;
const bool correct_EN;
const bool correct_FT;
const bool correct_AD;
Copilot is powered by AI and may make mistakes. Always verify output.
@tbellosta
Copy link
Contributor

Hi Nijso,
Nice of you to open a WIP PR. The branch feature_2ndOrderMixedGrids is still under development. The main addition are reported in the paper you linked and consist in a correction to the convective fluxes in order to recover second order spatial convergence on arbitrary hybrid grids. The branch also deals with boundary conditions which are now computed as a loop over marker faces in such a a way to integrate exactly a linear flux (thus preserving second order convergence, if the surface normals are "good enough"). That reguarding BCs is probably the deeper change to the codebase, and would require some thinking if we plan to eventually merge the feature branch.
Other additions that are reported in the paper (and available in the branch) are:

  • more accurate Roe Jacobians (i.e. we added the jacobian of the dissipation matrix, which was missing), viscous flux Jacobians, and BC jacobians (added the derivative of the external state w.r.t. the internal state in e.g. the farfield BCs). Now the Roe Jacobian should be exact for a first order scheme, and the viscous jacobian is exact as well for zero LSQ gradients.
  • Alternative definition of cell centroid that works well with triangulated BL meshes (would loose 2nd order without the flux correction procedure)
  • Added different corrections for the interface gradient employed in the viscous flux computation i.e. edge-normal (that's whats already there in the code), face-tangent, and alpha-damping.

I will keep you up-to-date in this thread and I will open a more thorough PR when the branch feature_2ndOrderMixedGrids is ready (I have some restructuring/clean-up to do as well as additional tests to run)

@pcarruscag
Copy link
Member

Hey Tommaso,
Those are all very exciting developments.
Please consider contributing them to the main branch in incremental steps, you have many independent changes in that branch, it would be best practice to test the effects of each of them incrementally. That way, if a bug shows up, it's also a lot easier to track it down.
And if there are questions about the best way of integrating intrusive changes, I would be very happy to help.

@bigfooted
Copy link
Contributor Author

Hi @tbellosta, I started this PR to get things rolling again, because development seemed to have stalled and I think these are great developments that can really push the accuracy and robustness forward.
I think your developments might also be a solution to problems like discussed here currently:
#2576

To make it easier to get these developments into develop, I suggest to separate the 4(?) main ideas: this defect correction, the flux correction, the control volume computation and the modified Roe scheme as was also mentioned by Pedro.

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