Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) #1392

WallyMaier · 2021-10-06T05:54:28Z

Proposed Changes

This draft PR aims to generalize and template the turbulence solver/ scalar solvers to reduce code duplication and allow NEMO access to the turbulence solvers. The ideas were discussed with @pcarruscag, @TobiKattmann and @suargi,
I am opening up the draft PR very early, so this is very much a work in process.

The main idea is use templates to introduce differing variable index values into the Scalar/Turb solvers.

Related Work

This idea follows PR #1375 and PR #1330

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 the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
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.

SU2_CFD/include/variables/CEulerVariable.hpp

…lent

…bulent

lgtm-com · 2021-11-07T13:34:51Z

This pull request fixes 1 alert when merging ac3f474 into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

lgtm-com · 2021-11-07T16:21:47Z

This pull request fixes 1 alert when merging 9790d11 into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

lgtm-com · 2021-11-07T17:37:47Z

This pull request fixes 1 alert when merging 60eed7c into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

lgtm-com · 2021-11-07T23:51:45Z

This pull request fixes 1 alert when merging 40c5586 into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

pcarruscag · 2021-11-08T09:02:52Z

SU2_CFD/include/numerics/turbulent/turb_diffusion.hpp

+      nu_e = nu_ij + fn*nu_tilde_ij;
+    }
+
+    Flux[0] = nu_e*Proj_Mean_GradScalarVar[0]/sigma;


There was a bug here for SA-neg as it was using the "uncorrected flux" which is prone to odd-even decoupling.
To keep track of this I've changed the scope of the PR.

The intrusive part of the changes (for @TobiKattmann and @suargi ) is done, so if it is ok with you I propose merging this now, to use it also in the species solver and refactored SA sources.

@WallyMaier please continue the NEMO turbulence in another PR, it should work out of the box now.

I'll give this PR a read later today 👍

pcarruscag · 2021-11-08T09:08:51Z

SU2_CFD/src/numerics/turbulent/turb_sources.cpp

-    Density_i = V_i[nDim+2];
-    Laminar_Viscosity_i = V_i[nDim+5];
-  }
+  Density_i = V_i[idx.Density()];


I kept the sources cpp instead of moving to the hpp only to highlight the changes for @suargi , eventually it should be in an hpp to avoid the explicit instantiations (bottom of the file).

lgtm-com · 2021-11-08T13:25:17Z

This pull request fixes 1 alert when merging f7d054e into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

WallyMaier

@pcarruscag thanks for taking this over. It was much needed and I was dragging my feet. Ill work to make the necessary NEMO changes in another PR. I can't approved but LGTM.

WallyMaier · 2021-11-08T14:36:52Z

SU2_CFD/include/numerics/scalar/scalar_convection.hpp

+      }
+    } else {
+      for (unsigned short iDim = 0; iDim < nDim; iDim++) {
+        q_ij += 0.5 * (V_i[iDim + idx.Velocity()] + V_j[iDim + idx.Velocity()]) * Normal[iDim];


WallyMaier · 2021-11-08T14:44:02Z

SU2_CFD/src/variables/CNEMOEulerVariable.cpp

@@ -41,6 +41,7 @@ CNEMOEulerVariable::CNEMOEulerVariable(su2double val_pressure,
                                       const CConfig *config,
                                       CNEMOGas *fluidmodel)
  : CFlowVariable(npoint, ndim, nvar, nvarprim, nvarprimgrad, config),
+    indices(ndim, config->GetnSpecies()),


WallyMaier · 2021-11-08T14:49:31Z

TestCases/serial_regression.py

@@ -355,7 +355,7 @@ def main():
    turb_naca0012_sa.cfg_dir   = "rans/naca0012"
    turb_naca0012_sa.cfg_file  = "turb_NACA0012_sa.cfg"
    turb_naca0012_sa.test_iter = 10
-    turb_naca0012_sa.test_vals = [-11.133933, -14.498178, 1.064330, 0.019756, 20, -1.898609, 20, -2.719825]
+    turb_naca0012_sa.test_vals = [-8.629583, -10.377793, 1.064488, 0.019711, 20.000000, -2.173971, 20.000000, -5.213344]


Any idea why plain old SA would see such a change?

it's SA-neg for which the bug was fixed

It seems that the regression tests introduced in #1418 have passed, in particular in parallel_regression.py therer is a regression test for the negative SA. This means that this regression test is not properly defined and should be improved. Wonder about the others.

TobiKattmann

Thanks @WallyMaier and @pcarruscag for this. No more guessing what e.g V_i[nDim+2] now actually is 👍

I think the changes I have to make to #1388 are somewhat clear (after all, I can follow the Turbulence example :) )

So LGTM

TobiKattmann · 2021-11-08T15:22:46Z

SU2_CFD/include/numerics/scalar/scalar_convection.hpp

+  su2double Flux[MAXNVAR];          /*!< \brief Final result, diffusive flux/residual. */
+  su2double* Jacobian_i[MAXNVAR];   /*!< \brief Flux Jacobian w.r.t. node i. */
+  su2double* Jacobian_j[MAXNVAR];   /*!< \brief Flux Jacobian w.r.t. node j. */
+  su2double JacobianBuffer[2*MAXNVAR*MAXNVAR];  /*!< \brief Static storage for the two Jacobians. */


The great static-ification of SU2 👍

SU2_CFD/include/numerics/scalar/scalar_convection.hpp

SU2_CFD/include/numerics/turbulent/turb_convection.hpp

SU2_CFD/include/numerics/turbulent/turb_diffusion.hpp

SU2_CFD/include/variables/CEulerVariable.hpp

SU2_CFD/src/drivers/CDriver.cpp

SU2_CFD/src/numerics/scalar/scalar_convection.cpp

SU2_CFD/src/numerics/turbulent/turb_sources.cpp

Co-authored-by: TobiKattmann <31306376+TobiKattmann@users.noreply.github.com>

lgtm-com · 2021-11-08T17:35:23Z

This pull request fixes 1 alert when merging 57a736a into 147dfac - view on LGTM.com

fixed alerts:

1 for Comparison of narrow type with wide type in loop condition

pcarruscag · 2021-11-08T23:22:49Z

Thank you for the reviews folks, no feet were dragged, I was just looking for a relaxed Sunday activity.

Introduced in #1392. The porting approach for now was to simply to way it was done for turbulence. For the species_sources one could and should prob adopt the approach for convection/diffusion. But for ease of merging that was not done yet.

pcarruscag · 2021-11-09T10:13:54Z

TestCases/parallel_regression.py

@@ -641,7 +641,7 @@ def main():
    turbmod_sa_neg_rae2822.cfg_dir   = "turbulence_models/sa/rae2822"
    turbmod_sa_neg_rae2822.cfg_file  = "turb_SA_NEG_RAE2822.cfg"
    turbmod_sa_neg_rae2822.test_iter = 20
-    turbmod_sa_neg_rae2822.test_vals = [-2.004688, 0.742320, 0.497307, -5.265640, 0.809478, 0.061986]
+    turbmod_sa_neg_rae2822.test_vals = [-2.004689, 0.742306, 0.497308, -5.265793, 0.809463, 0.062016]
    turbmod_sa_neg_rae2822.su2_exec  = "mpirun -n 2 SU2_CFD"


It did not pass @suargi

My bad. Thanks!

initial commit towards generalized solver indices+ nemo turb

294deb1

pr-triage bot added the PR: draft label Oct 6, 2021

WallyMaier changed the base branch from master to develop October 6, 2021 05:54

su2code deleted a comment from lgtm-com bot Oct 6, 2021

pcarruscag changed the title ~~[WIP] Generalized Turbulent solver indices -> NEMO turbulence~~ [WIP] NEMO Turbulence (generalize indices of flow variables for use by scalar solvers and numerics) Oct 9, 2021

pcarruscag added the changelog:feature label Oct 9, 2021

pcarruscag reviewed Oct 9, 2021

View reviewed changes

SU2_CFD/include/variables/CEulerVariable.hpp Outdated Show resolved Hide resolved

pcarruscag and others added 7 commits October 9, 2021 11:38

Update SU2_CFD/include/variables/CEulerVariable.hpp

77dcc80

Merge remote-tracking branch 'origin/develop' into feature_NEMO_turbu…

f6dfac8

…lent

Update incomp and nemo index structure

bb35691

Merge remote-tracking branch 'upstream/develop' into feature_NEMO_tur…

4b604b3

…bulent

put the code in a compilable state

3213f85

use the index classes in the variables

15a6915

using the indices in scalar convection

ac3f474

pcarruscag mentioned this pull request Nov 7, 2021

Species transport solver (passive for now) #1388

Merged

5 tasks

viscous scalar and turbulent numerics

9790d11

fix old bug in SA-neg diffusion and abstract "velocity gradient"

60eed7c

pcarruscag added 2 commits November 7, 2021 22:03

update regressions after bug fix

36282d7

turbulence sources

40c5586

pcarruscag changed the title ~~[WIP] NEMO Turbulence (generalize indices of flow variables for use by scalar solvers and numerics)~~ Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) Nov 8, 2021

pcarruscag marked this pull request as ready for review November 8, 2021 08:54

pr-triage bot added PR: unreviewed and removed PR: draft labels Nov 8, 2021

pcarruscag added changelog:fix and removed changelog:feature labels Nov 8, 2021

pcarruscag reviewed Nov 8, 2021

View reviewed changes

doxygen

f7d054e

WallyMaier commented Nov 8, 2021

View reviewed changes

TobiKattmann approved these changes Nov 8, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Nov 8, 2021

Apply suggestions from code review

57a736a

Co-authored-by: TobiKattmann <31306376+TobiKattmann@users.noreply.github.com>

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Nov 8, 2021

pcarruscag merged commit 641d254 into develop Nov 8, 2021

pcarruscag deleted the feature_NEMO_turbulent branch November 8, 2021 23:23

pr-triage bot added PR: merged PR: unreviewed and removed PR: unreviewed PR: merged labels Nov 8, 2021

pcarruscag reviewed Nov 9, 2021

View reviewed changes

pcarruscag mentioned this pull request Dec 24, 2021

Using the primitive variable index classes in more places (+ minor chores) #1476

Merged

pcarruscag mentioned this pull request Jan 14, 2022

C++ Core Guidelines #1218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) #1392

Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) #1392

WallyMaier commented Oct 6, 2021 •

edited

Loading

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

pcarruscag Nov 8, 2021

TobiKattmann Nov 8, 2021

pcarruscag Nov 8, 2021

lgtm-com bot commented Nov 8, 2021

WallyMaier left a comment •

edited

Loading

WallyMaier Nov 8, 2021

WallyMaier Nov 8, 2021

WallyMaier Nov 8, 2021

pcarruscag Nov 8, 2021

suargi Nov 9, 2021

TobiKattmann left a comment

TobiKattmann Nov 8, 2021

lgtm-com bot commented Nov 8, 2021

pcarruscag commented Nov 8, 2021

pcarruscag Nov 9, 2021

suargi Nov 9, 2021

Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) #1392

Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) #1392

Conversation

WallyMaier commented Oct 6, 2021 • edited Loading

Proposed Changes

Related Work

PR Checklist

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

lgtm-com bot commented Nov 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 8, 2021

WallyMaier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiKattmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 8, 2021

pcarruscag commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WallyMaier commented Oct 6, 2021 •

edited

Loading

WallyMaier left a comment •

edited

Loading