Compressible CHT fixes #839

oleburghardt · 2019-12-12T13:01:21Z

Proposed Changes

This PR includes a fix that will reenable CHT for compressible flow (unintentionally out of service in v7 so far). A regression test was added to protect this functionality together with some general CHT-related cleanups.

Related Work

Fixes issue #838

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

…pressible_CHT

pcarruscag

LGTM, I think what is proposed in #770 could help prevent this kind of bugs.

oleburghardt · 2019-12-19T13:00:06Z

LGTM, I think what is proposed in #770 could help prevent this kind of bugs.

Probably it could help as one could painlessly add special entries to such a bit field that will remove the ||'s from the code, even for minor examples like this.

I'll leave this as draft for a while because I'd like to test some more compressible stuff in terms of reference values, non-dimensionalization and so on. It's a good opportunity.

…pressible_CHT

talbring · 2020-01-07T14:57:21Z

SU2_CFD/src/solver_direct_mean.cpp

+      // feasible alternative to compute (conjugate) wall termperature
+      // HF_FactorHere = thermal_conductivity*config->GetViscosity_Ref()/dist_ij;
+      // HF_FactorConjugate = GetConjugateHeatVariable(val_marker, iVertex, 2);
+      // Twall = (There*HF_FactorHere + Tconjugate*HF_FactorConjugate)/(HF_FactorHere + HF_FactorConjugate);


If that's feasible consider adding it as an option, otherwise remove it

…pressible_CHT

…led CHT case.

…into fix_compressible_CHT

… an option.

oleburghardt · 2020-01-17T14:36:16Z

SU2_CFD/src/solver_direct_mean.cpp

+      if ((config->GetKind_CHT_Coupling() == AVERAGED_TEMPERATURE_NEUMANN_HEATFLUX)
+          || (config->GetKind_CHT_Coupling() == AVERAGED_TEMPERATURE_ROBIN_HEATFLUX)) {

-      // this will be changed soon...
-      Twall = GetConjugateHeatVariable(val_marker, iVertex, 0);
+        HF_FactorHere = thermal_conductivity*config->GetViscosity_Ref()/dist_ij;
+        HF_FactorConjugate = GetConjugateHeatVariable(val_marker, iVertex, 2);
+        Twall = (There*HF_FactorHere + Tconjugate*HF_FactorConjugate)/(HF_FactorHere + HF_FactorConjugate);
+      }
+      else {

-      dTdn = -(There - Twall)/dist_ij;
+        Twall = Tconjugate;
+        dTdn = -(There - Twall)/dist_ij;
+      }


@talbring Please check :) Let me know before you merge this in, I'm still checking for some performance differences between those methods for the coupled_cht test case. (To determine which one should be initialised as default.)

TobiKattmann

Hi @oleburghardt ,
Thanks for the fix and the Testcase. I have some questions below and I would like to understand the implications for my work 😉

I don't want to go beyond the scope of this PR, but if you like, the off-PR comments can be integrated in this PR to clear that up a bit. If not that is OK as well
A bit off-PR: in solver_direct_heat:cppBC_ConjugateHeat_Interface the input value val_marker is not really used... there are loops over iMarker which seem unnecessary.
(Also in BC_HeatFlux_Wall I guess the iMarker_HeatFlux loop can be omitted -> from what is in integration_structure.cpp at case CHT_WALL_INTERFACE i figured that this BC is called for inc+weakly-coupled heat. Here I would prefer as well integrating it in BC_ConjugateHeat_Interface, or writing a short hint in both BCs)

Common/src/config_structure.cpp

SU2_CFD/src/solver_direct_heat.cpp

TobiKattmann · 2020-01-20T10:15:27Z

SU2_CFD/src/solver_direct_mean.cpp

+      if ((config->GetKind_CHT_Coupling() == AVERAGED_TEMPERATURE_NEUMANN_HEATFLUX)
+          || (config->GetKind_CHT_Coupling() == AVERAGED_TEMPERATURE_ROBIN_HEATFLUX)) {

-      // this will be changed soon...
-      Twall = GetConjugateHeatVariable(val_marker, iVertex, 0);
+        HF_FactorHere = thermal_conductivity*config->GetViscosity_Ref()/dist_ij;
+        HF_FactorConjugate = GetConjugateHeatVariable(val_marker, iVertex, 2);
+        Twall = (There*HF_FactorHere + Tconjugate*HF_FactorConjugate)/(HF_FactorHere + HF_FactorConjugate);
+      }
+      else {


I would prefer here (i.e. instead of the else) to check config->GetKind_CHT_Coupling() explicitly against the options that are reasonable here and add if you like an } else { SU2_MPI::Error("Unknown Kind_CHT_Coupling..."). (Same goes for solver_direct_heat.cpp)

To give a reason why: I tend to grep through the repository quite often for keywords to get an overview and if an option is implicitly taken in an else one cannot find it directly. (One can always grep the Getter function of course)

Complicated "if ... Else if... Else if..." statements are harder for the compiler to optimize.

Hmm alright, I'll add the error message, it's a bit more safe..

@pcarruscag is a switch statement better, or what is the best way to resolve that? In this case it is only if..else if..else.

Mind you that most times this is not a super important optimization, I do show a case in 789 where it is.
But anyway, ideally error conditions will have been handled upstream and one does not need to include error handling code in hot regions (i.e. loops) where it would pollute the instruction cache. In which case if the purpose is to document just add what the "else" implies as a comment. Switches are for "out of many options choose one kind of deal".
The reason why simple if/else code might work better is not (just) the reduction of logical checks, as within a loop the cpu will start predicting the right branch to take. The real advantage is if instead of branching the compiler manages to use conditional move instructions instead.

There goes another day me figuring and trying out what conditional move instructions are 🙈
I actually wanted to argue with branch prediction that it shouldn't matter that much.... but now I can learn sth new 📚

Common/include/option_structure.hpp

SU2_CFD/src/solver_direct_mean.cpp

Common/include/config_structure.inl

…lver.

oleburghardt · 2020-01-22T10:12:33Z

the off-PR comments can be integrated in this PR to clear that up a bit. If not that is OK as well

It's more than OK, the CHT implementation suffers a bit from being a single person's project.. you're very welcome to have a look over it! :-)

Can you add that option to the config_template.cfg together with that description, the valid inputs and the default

Actually I'm hesitating a bit adding it to the config template right now. I'd prefer to have DIRECT_TEMPERATURE_ROBIN_HEATFLUX as the default and "hide" the rest as developer's options for the moment.
The reason simply is that those different methods need to checked and validated against each other before we make them public.
E.g. I figured that for the incompressible CHT test case from the repo, there is a severe gap between the heatfluxes obtained from the "averaged" approach and the direct one, see below.

That needs to be cleared up first...

TobiKattmann · 2020-01-22T12:52:55Z

I'd prefer to have DIRECT_TEMPERATURE_ROBIN_HEATFLUX as the default and "hide" the rest as developer's options for the moment.

Ok, makes sense if the results differ and need some more testing to not add it to the config_template.

you're very welcome to have a look over it! :-)

I'll do my very best 👴

oleburghardt · 2020-01-23T09:43:34Z

Also in BC_HeatFlux_Wall I guess the iMarker_HeatFlux loop can be omitted

The loop is just added because the surface areas are not stored per maker but just by the heat flux markers.

i figured that this BC is called for inc+weakly-coupled heat

Not really, it's called when there is no heat solver at all. (Being a no-slip BC then.)

…pressible_CHT

TobiKattmann · 2020-01-23T18:27:07Z

Hey @oleburghardt , thanks again for the fix and for the clean-up bits. From my side this is good to go 🌬 ⛵️

pcarruscag · 2020-01-24T18:41:19Z

SU2_CFD/src/interfaces/cht/CConjugateHeatInterface.cpp

+  bool incompressible_flow = ((donor_config->GetKind_Solver() == INC_NAVIER_STOKES)
               || (donor_config->GetKind_Solver() == INC_RANS)
               || (donor_config->GetKind_Solver() == DISC_ADJ_INC_NAVIER_STOKES)
-               || (donor_config->GetKind_Solver() == DISC_ADJ_INC_RANS));
-
-  bool compressible_flow    = (donor_config->GetKind_Regime() == COMPRESSIBLE) && flow;
-  bool incompressible_flow  = (donor_config->GetEnergy_Equation()) && flow;
-  bool heat_equation        = (donor_config->GetKind_Solver() == HEAT_EQUATION_FVM
-                               || donor_config->GetKind_Solver() == DISC_ADJ_HEAT);
+               || (donor_config->GetKind_Solver() == DISC_ADJ_INC_RANS)
+               && (donor_config->GetEnergy_Equation()));


@oleburghardt you ticked the "no new compiler warnings" box, but there is a warning for this logic and it does look a bit suspicious.

Thanks Pedro, I'll check 👍

oleburghardt added 2 commits December 12, 2019 12:43

Fix including compressible case to CHT interface initialization.

5bd51a6

Merge branch 'develop' of https://github.com/su2code/SU2 into fix_com…

7866c0f

…pressible_CHT

pr-triage bot added the PR: draft label Dec 12, 2019

oleburghardt added the changelog:fix label Dec 12, 2019

Small fix.

14d83c8

pcarruscag approved these changes Dec 14, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: draft labels Dec 14, 2019

Slight cleanup of compressible CHT BC routine.

cd45b2f

pr-triage bot added PR: draft and removed PR: reviewed-approved labels Jan 6, 2020

oleburghardt added 4 commits January 6, 2020 10:07

Merge branch 'develop' of https://github.com/su2code/SU2 into fix_com…

f479d75

…pressible_CHT

Add compressible CHT case to TestCases.

d8e0acc

Change iteration number in compressible CHT test case.

57eeebb

Add compressible CHT test case to regression tests.

1ee0fb0

oleburghardt marked this pull request as ready for review January 6, 2020 10:34

pr-triage bot added PR: unreviewed and removed PR: draft labels Jan 6, 2020

Update parallel_regression.py

604fcd0

talbring reviewed Jan 7, 2020

View reviewed changes

oleburghardt added 4 commits January 17, 2020 13:38

Merge branch 'develop' of https://github.com/su2code/SU2 into fix_com…

1d31b8e

…pressible_CHT

Provide gradient value for validation in adjoint config file for coup…

b0e22d6

…led CHT case.

Merge branch 'fix_compressible_CHT' of https://github.com/su2code/SU2 …

a889f5d

…into fix_compressible_CHT

Turn CHT coupling boolean into list of methods; make uncommented code…

409485b

… an option.

oleburghardt commented Jan 17, 2020

View reviewed changes

TobiKattmann requested changes Jan 20, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jan 20, 2020

Some small changes for CHT routines.

fd9cbe6

oleburghardt added 3 commits January 20, 2020 17:32

Remove unintended loop over markers from heat solver CHT BC routine.

2487090

Include averaged temperature CHT coupling method to incompressible so…

48ec551

…lver.

CHT-related fix.

e6f78b8

pr-triage bot added PR: unreviewed and removed PR: reviewed-changes-requested labels Jan 20, 2020

Add small comment.

2bd22d0

oleburghardt added 2 commits January 23, 2020 10:44

Add some safety error messages in CHT-BC routines.

42d15f8

Merge branch 'develop' of https://github.com/su2code/SU2 into fix_com…

03db443

…pressible_CHT

TobiKattmann approved these changes Jan 23, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 23, 2020

talbring merged commit 00f47ab into develop Jan 24, 2020

talbring deleted the fix_compressible_CHT branch January 24, 2020 12:37

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Jan 24, 2020

pcarruscag reviewed Jan 24, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: merged labels Jan 24, 2020

pcarruscag mentioned this pull request Jan 25, 2020

Restructuring of solver files #849

Merged

5 tasks

Compressible CHT fixes #839

Compressible CHT fixes #839

Uh oh!

Conversation

oleburghardt commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

oleburghardt commented Dec 19, 2019

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleburghardt commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiKattmann commented Jan 22, 2020

Uh oh!

oleburghardt commented Jan 23, 2020

Uh oh!

TobiKattmann commented Jan 23, 2020

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!

5 participants

oleburghardt commented Dec 12, 2019 •

edited

Loading

oleburghardt commented Jan 22, 2020 •

edited

Loading