Improve the configuration file for the SA Neg test case #1559

suargi · 2022-03-10T15:03:15Z

Proposed Changes

Improve the configuration file for the turbulence_models SA Neg test case. With this new config file we should be better able to detect bugs in the code. For instance, with this new config file and number of iterations for the regression test, we will be able to detect bd40210.

PR Checklist

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.

TobiKattmann · 2022-03-15T11:17:55Z

TestCases/parallel_regression.py

@@ -673,8 +673,8 @@ def main():
    turbmod_sa_neg_rae2822           = TestCase('turbmod_sa_neg_rae2822')
    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.004689, 0.742306, 0.497308, -5.265793, 0.809463, 0.062016]
+    turbmod_sa_neg_rae2822.test_iter = 10


I am curious why a reduced number of iterations would be beneficial? I would always tend to do more as small errors might accumulate enough only after a certain amount of iterations, to break the regression test. That scenario might not happen to frequently but as you also remove multigrid there should be some time freed-up, right :)

So feel free (from my point of view) to revert to 20 or higher if it doesnt cost minutes.

TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg

TobiKattmann · 2022-03-15T11:20:02Z

TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg

@@ -14,7 +14,7 @@ RESTART_SOL= NO
 MACH_NUMBER= 0.729
 %
 % Angle of attack (degrees, only for compressible flows)
-AOA= 2.31
+AOA= 20.31


I recon, the case still converges to a reasonable solution with that more aggressive AoA?

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

suargi · 2022-03-15T12:24:10Z

Thank you @TobiKattmann for your feedback.

The idea behind this new regression test config file is as follows:
We should have a test case that triggers the negative part of the SA model. Depending on the flow conditions, geometry and CFD parameters it might be triggered or not. With the previous config file, it was not.
I think there is already a regression test in SU2 that triggers the negative SA, the turb_oneram6_nk. However, I decided to stick to rae2822 airfoil as it is a simpler (faster) case. In order to force the negative part of the SA for the rae2822 in a reasonable amount of iterations for a regression test, I increased the angle of attack. I have removed the multigrid as it might not be stable, but I have not tested though. Anyway, with the current configuration, e.g., convective scheme, CFL number, etc, the solution is not stable and diverges after some iterations, around 15. In that sense, I reduced the number of iterations from 20 to 10.

In my opinion, a diverging regression test is not a problem at all as it might not be used as a tutorial, only to verify the integrity of the commit. "The solution should always diverge to the same results". If the regression test should converge, let me know and I will update the config file :)

pcarruscag · 2022-03-15T12:33:22Z

That is a bit of a necessary evil, I would leave a warning in the config explaining exactly what you just did.

TobiKattmann · 2022-03-15T13:16:56Z

Thanks for the explanation @suargi . I would personally advocate for that the testcases should converge to some reasonable solution people might use it as a starting point (copy the cfg and doing mild adaptions) for their own stuff. And the Testcases show off the capabilities to some degree, to do so, convergence is beneficial.

But as we have a bunch of working 2D airfoils in regression already I recon that adding a clear explanation and warning to the cfg as suggested by Pedro is fine. Otherwise you might try to bisect the AoA ... maybe there is a value that triggers negative SA and does not diverge 🤔

Knowingly adding a diverging test without a clear warning is not good imo :)

Include a header warning explaining the purpose and how to use this testcase.

suargi · 2022-03-15T16:03:08Z

Header with a warning included in commit 017616f.

suargi · 2022-03-17T07:54:09Z

Is it ready to merge?

TobiKattmann · 2022-03-17T08:59:13Z

TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg

+%                      This regression test will diverge!                      %
+%									       %
+% This test case is only meant to trigger the negative part of the SA model,   %
+% to check the integrity of the commits. It is inadvisable to use it as an     %
+% starting point for tutorials.						       %
+% The rae2822 airfoil is a simple (fast) case. A high angle of attack is being %


For some reason github Files changed tab shows me a messed up indentation ... but looking at the actual file the indentation is on point. Not sure why github does that 🤷

hmm ... in chromium that looks good 🤔

TobiKattmann · 2022-03-17T09:08:21Z

Is it ready to merge?

From my side yes :) Can you though make sure the reg test run and report here in the PR correctly. I am not really what happened there. I started a re-run of all the jobs so if that runs like usual feel free to merge 👍

TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg

suargi · 2022-03-17T10:33:50Z

I do not know why a regression test from hybrid_regression_AD.py has failed. But it has nothing to do with this pull request. Therefore I think we can merge it.

TobiKattmann · 2022-03-17T10:55:33Z

I do not know why a regression test from hybrid_regression_AD.py has failed. But it has nothing to do with this pull request. Therefore I think we can merge it.

Yep sometimes certain cases of hybrid-reg-ad fail ... sometimes they don't :)
That is why they are not a hard requirement for a merge. Usually I take a look nonetheless to not overlook sth.
So feel free to merge 👍

Improve the configuration file for the SA Neg test case

7cd5426

pr-triage bot added the PR: draft label Mar 10, 2022

Update test values and iterations

3b74901

suargi removed the PR: draft label Mar 15, 2022

suargi marked this pull request as ready for review March 15, 2022 08:41

pr-triage bot added the PR: unreviewed label Mar 15, 2022

TobiKattmann approved these changes Mar 15, 2022

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Mar 15, 2022

TobiKattmann added the changelog:chore label Mar 15, 2022

Update TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg

1220dfc

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

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Mar 15, 2022

Header warning

017616f

Include a header warning explaining the purpose and how to use this testcase.

suargi and others added 2 commits March 16, 2022 08:58

Merge branch 'develop' into fix_saneg_variant_testcase

ff22728

Merge branch 'develop' into fix_saneg_variant_testcase

6d7dffd

TobiKattmann reviewed Mar 17, 2022

View reviewed changes

TestCases/turbulence_models/sa/rae2822/turb_SA_NEG_RAE2822.cfg Outdated Show resolved Hide resolved

Small change to trigger regression tests.

f749e52

suargi merged commit 4c3311f into develop Mar 17, 2022

suargi deleted the fix_saneg_variant_testcase branch March 17, 2022 10:57

pr-triage bot removed the PR: unreviewed label Mar 17, 2022

pr-triage bot added the PR: merged label Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the configuration file for the SA Neg test case #1559

Improve the configuration file for the SA Neg test case #1559

suargi commented Mar 10, 2022 •

edited

Loading

TobiKattmann Mar 15, 2022

TobiKattmann Mar 15, 2022

suargi commented Mar 15, 2022

pcarruscag commented Mar 15, 2022

TobiKattmann commented Mar 15, 2022

suargi commented Mar 15, 2022

suargi commented Mar 17, 2022

TobiKattmann Mar 17, 2022

TobiKattmann Mar 17, 2022

TobiKattmann Mar 17, 2022

TobiKattmann commented Mar 17, 2022

suargi commented Mar 17, 2022

TobiKattmann commented Mar 17, 2022

Improve the configuration file for the SA Neg test case #1559

Improve the configuration file for the SA Neg test case #1559

Conversation

suargi commented Mar 10, 2022 • edited Loading

Proposed Changes

PR Checklist

TobiKattmann Mar 15, 2022

Choose a reason for hiding this comment

TobiKattmann Mar 15, 2022

Choose a reason for hiding this comment

suargi commented Mar 15, 2022

pcarruscag commented Mar 15, 2022

TobiKattmann commented Mar 15, 2022

suargi commented Mar 15, 2022

suargi commented Mar 17, 2022

TobiKattmann Mar 17, 2022

Choose a reason for hiding this comment

TobiKattmann Mar 17, 2022

Choose a reason for hiding this comment

TobiKattmann Mar 17, 2022

Choose a reason for hiding this comment

TobiKattmann commented Mar 17, 2022

suargi commented Mar 17, 2022

TobiKattmann commented Mar 17, 2022

suargi commented Mar 10, 2022 •

edited

Loading