Fix bug in SA-neg diffusion term (and generalize indices of flow variables for use by scalar solvers and numerics) by WallyMaier · Pull Request #1392 · su2code/SU2

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.

…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

+      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

-    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

+      }
+    } 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

                                       const CConfig *config,
                                       CNEMOGas *fluidmodel)
  : CFlowVariable(npoint, ndim, nvar, nvarprim, nvarprimgrad, config),
+    indices(ndim, config->GetnSpecies()),


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

    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

+  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 👍

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

    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

Comment thread SU2_CFD/include/variables/CEulerVariable.hpp Outdated

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

Uh oh!

Conversation

WallyMaier commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 7, 2021

Uh oh!

lgtm-com Bot commented Nov 7, 2021

Uh oh!

lgtm-com Bot commented Nov 7, 2021

Uh oh!

lgtm-com Bot commented Nov 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com Bot commented Nov 8, 2021

Uh oh!

WallyMaier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Nov 8, 2021

Uh oh!

pcarruscag commented Nov 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WallyMaier commented Oct 6, 2021 •

edited

Loading

WallyMaier left a comment •

edited

Loading